Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for specifying --host via environment variable #4734

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

maouw
Copy link
Contributor

@maouw maouw commented Mar 18, 2024

What does this PR change? What problem does it solve?

This PR adds the ability to set the hostname for most commands that accept the --host flag via the RESTIC_HOST environment variable. This allows the user to set the hostname once and not have to specify it for every command. Commands that accept --host but do not work on snapshots (e.g. restic key ...) are not affected by this change. The --host flag is still accepted and takes precedence over the RESTIC_HOST environment variable.

The changes are in cmd_backup.go for the backup command and find.go for commands that use the restic.SnapshotFilter options.

Was the change previously discussed in an issue or on the forum?

Closees #4733

Checklist

  • I have read the contribution guidelines.
  • I have enabled maintainer edits.
  • I have added tests for all code changes.
  • I have added documentation for relevant changes (in the manual).
  • There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
  • I have run gofmt on the code in all commits.
  • All commit messages are formatted in the same style as the other commits in the repo.
  • I'm done! This pull request is ready for review.

@maouw
Copy link
Contributor Author

maouw commented Mar 18, 2024

I have a question to the maintainers -- where and/or how would you suggest putting the proper tests for this feature? Most of the tests related to this area seem to circumvent option processing, so I can't find a good example to follow. I am a (relative) beginner to golang.

Copy link
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I only have a few comments, see below.

The best way for testing is probably to use something like the following. That simply creates a new flagSet, configures it, then parses a test string and verifies the outcome:

set := pflag.NewFlagSet("test", pflag.PanicOnError)
flt := &restic.SnapshotFilter{}
initMultiSnapshotFilter(set, flt, false)
err := set.Parse([]string{"--host", "abc"})
rtest.OK(t, err)
// assert on flt.Hosts

changelog/unreleased/issue-4733 Outdated Show resolved Hide resolved
cmd/restic/find.go Outdated Show resolved Hide resolved
@MichaelEischer MichaelEischer linked an issue Apr 4, 2024 that may be closed by this pull request
This commit adds support for specifying the `--host` option via the `RESTIC_HOST` environment variable. This is done by extending option processing in `cmd_backup.go` and for `restic.SnapshotFilter` in `find.go`.
Copy link
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I've applied my suggestions and added a short test.

@MichaelEischer MichaelEischer added this pull request to the merge queue Apr 24, 2024
Merged via the queue into restic:master with commit faffd15 Apr 24, 2024
13 checks passed
@maouw
Copy link
Contributor Author

maouw commented Apr 30, 2024

Thank you for taking care of the test. Very happy to see this coming through, it will be a great help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set --host via environment variable
2 participants