- Notifications
You must be signed in to change notification settings - Fork919
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
fix(config): allow usage of full config struct#1683
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 fromgo-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 theinit() function were added ind3d0267. The last code usinglog was removed inebd153f. 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 bygo-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 astautological condition: non-nil != nil bynilness.
Signed-off-by: Nicklas Frahm <nicklas.frahm@gmail.com>
b5f751c to9d7b45cCompareI 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.