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

fix(config): allow usage of full config struct #1683

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nicklasfrahm
Copy link

@nicklasfrahm nicklasfrahm commented Nov 24, 2024

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.

Comment on lines -27 to -31
var log *logrus.Logger

func init() {
log = logging.NewLogger("CONFIG")
}
Copy link
Author

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.

Copy link
Contributor

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)
Copy link
Author

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.

Copy link
Contributor

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.

Comment on lines -380 to -392
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)
}
Copy link
Author

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>
@nicklasfrahm
Copy link
Author

I am converting this to a draft until we reach a conclusion in the GitHub issue.

@nicklasfrahm nicklasfrahm marked this pull request as draft November 25, 2024 21:56
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.

Expose full config via Go API
2 participants