feat: enhance environment variable handling in NewRepo, allow the rep…

[adrian-taralunga]

feat: enhance environment variable handling in NewRepo, allow the repo uri to have envirnonment variables, prioritising first the ones from opts and then adding if any missing from the os environment

[garethgeorge]

Thanks for the interest in contributing, generally open to supporting more expansions with some caveats --

I've been hesitant to blanket expand $ENV syntax as is done in shells as this may conflict with $ in a user's password or in an env var value or secret-- I don't want people to have to commonly escape values. I've settled at the moment for substituting anything of the form ${VAR} as the only supported substitution format.

The existing env var expansion is implemented here

backrest/internal/orchestrator/repo/env.go

Line 13 in 3ab6271

func ExpandEnv(s string) string {

But isn't applied in as many places as it could be, env var substitution actually happens a layer above the package you're looking at. It happens here

backrest/internal/orchestrator/repo/repo.go

Lines 36 to 83 in 3ab6271

	func NewRepoOrchestrator(config *v1.Config, repoConfig *v1.Repo, resticPath string) (*RepoOrchestrator, error) {
	if config.Instance == "" {
	return nil, errors.New("instance is a required field in the backrest config")
	}
	var opts []restic.GenericOption
	if p := repoConfig.GetPassword(); p != "" {
	opts = append(opts, restic.WithEnv("RESTIC_PASSWORD="+p))
	}
	opts = append(opts, restic.WithEnviron())
	if env := repoConfig.GetEnv(); len(env) != 0 {
	for _, e := range env {
	opts = append(opts, restic.WithEnv(ExpandEnv(e)))
	}
	}
	for _, f := range repoConfig.GetFlags() {
	args, err := shlex.Split(ExpandEnv(f))
	if err != nil {
	return nil, fmt.Errorf("parse flag %q for repo %q: %w", f, repoConfig.Id, err)
	}
	opts = append(opts, restic.WithFlags(args...))
	}
	// Resolve command prefix
	if extraOpts, err := resolveCommandPrefix(repoConfig.GetCommandPrefix()); err != nil {
	return nil, fmt.Errorf("resolve command prefix: %w", err)
	} else {
	opts = append(opts, extraOpts...)
	}
	// Add BatchMode=yes to sftp.args if it's not already set.
	if slices.IndexFunc(repoConfig.GetFlags(), func(a string) bool {
	return strings.Contains(a, "sftp.args")
	}) == -1 {
	opts = append(opts, restic.WithFlags("-o", "sftp.args=-oBatchMode=yes"))
	}
	repo := restic.NewRepo(resticPath, repoConfig.GetUri(), opts...)
	return &RepoOrchestrator{
	config: config,
	repoConfig: repoConfig,
	repo: repo,
	}, nil
	}

. The thought being that the restic package should avoid business logic and focus on the contract w/restic.

So the concrete changes I'd make are

• use ExpandEnv util linked
• apply ExpandEnv to the repo URI on this line https://github.com/garethgeorge/backrest/blob/3ab62710a1e2278ab93479d0204fd86d33d436dd/internal/orchestrator/repo/repo.go#L76C37-L76C47

[adrian-taralunga]

@garethgeorge thank you for you comment. this is my first time using go, so I really appreciate your directions. I created another pull request with the changes you recommended. I see it's working on my side.