-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(config): allow usage of full config struct#1683
fix(config): allow usage of full config struct#1683nicklasfrahm wants to merge 1 commit intogetsops:mainfrom
Conversation
This allows the usage of the full config struct in 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.
| 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.
b5f751c to
9d7b45c
Compare
|
I am converting this to a draft until we reach a conclusion in the GitHub issue. |