-
Notifications
You must be signed in to change notification settings - Fork 978
fix(config): allow usage of full config struct #1683
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
base: main
Are you sure you want to change the base?
Conversation
| var log *logrus.Logger | ||
|
|
||
| func init() { | ||
| log = logging.NewLogger("CONFIG") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a warning from go-staticcheck and I could not see this being used anywhere so I removed it to avoid the warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better move the unrelated changes to a separate PR. That will increase the chance that these get merged, and make reviewing simpler generally.
Regarding this specific change: log and the init() function were added in d3d0267. The last code using log was removed in ebd153f. So there's definitely no reason to keep this.
| err := yaml.Unmarshal(bytes, f) | ||
| if err != nil { | ||
| return fmt.Errorf("Could not unmarshal config file: %s", err) | ||
| return fmt.Errorf("could not unmarshal config file: %s", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was also flagged by go-staticcheck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a change that should go into a feature release and not into a bugfix release since it changes the observable behavior a bit and isn't a bugfix. Because of that I would keep it separate from the other changes.
| if dRule != nil { | ||
| if dRule.S3Bucket != "" && dRule.GCSBucket != "" && dRule.VaultPath != "" { | ||
| return nil, fmt.Errorf("error loading config: more than one destinations were found in a single destination rule, you can only use one per rule") | ||
| } | ||
| if dRule.S3Bucket != "" { | ||
| dest = publish.NewS3Destination(dRule.S3Bucket, dRule.S3Prefix) | ||
| } | ||
| if dRule.GCSBucket != "" { | ||
| dest = publish.NewGCSDestination(dRule.GCSBucket, dRule.GCSPrefix) | ||
| } | ||
| if dRule.VaultPath != "" { | ||
| dest = publish.NewVaultDestination(dRule.VaultAddress, dRule.VaultPath, dRule.VaultKVMountName, dRule.VaultKVVersion) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was flagged as tautological condition: non-nil != nil by nilness.
Signed-off-by: Nicklas Frahm <nicklas.frahm@gmail.com>
b5f751c to
9d7b45c
Compare
|
I am converting this to a draft until we reach a conclusion in the GitHub issue. |
This allows the usage of the full config
structin another Go program.Closes #1682.
I added comments to edits that were not directly related to this PR, but that I considered a reduction of technical debt.
All tests pass locally.