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

Support env variables for config (#111) #177

Merged
merged 7 commits into from
Oct 22, 2024

Conversation

nikonhub
Copy link
Contributor

closes : #111

We could have used os.Getenv() directly as default value for flag.String(). But the are multiple uses of isFlagSet() in the code, which would return false even if the variable is set through the env.

@kehoecj kehoecj added hacktoberfest 🎃 Hacktoberfest 2024 waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers hacktoberfest-accepted Valid PR Hacktoberfest PR labels Oct 11, 2024
@kehoecj
Copy link
Collaborator

kehoecj commented Oct 11, 2024

@nikonhub Thanks for the PR! Approach looks solid - please address the golangci-lint findings

@nikonhub
Copy link
Contributor Author

@kehoecj There is a gocyclo error. But I also see another PR opened with the same error on the same function. How should we handle this ?

cmd/validator/validator.go Outdated Show resolved Hide resolved
@kehoecj
Copy link
Collaborator

kehoecj commented Oct 21, 2024

@kehoecj There is a gocyclo error. But I also see another PR opened with the same error on the same function. How should we handle this ?

Ignore - after hacktoberfest I'll go back and do some refactoring to satisfy gocyclone

Copy link
Collaborator

@kehoecj kehoecj left a comment

Choose a reason for hiding this comment

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

Please add a section to the README.md describing how users can use environment variables in addition to or instead of the command line arguments.

Make sure it's clear that a command line var will override an env var

Tested functionality - everything looks good on that end!

@kehoecj kehoecj added pr-action-requested PR is awaiting feedback from the submitting developer and removed waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Oct 21, 2024
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@kehoecj kehoecj left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for the PR @nikonhub !

@kehoecj kehoecj removed the pr-action-requested PR is awaiting feedback from the submitting developer label Oct 22, 2024
@kehoecj kehoecj merged commit 677d92e into Boeing:main Oct 22, 2024
9 of 10 checks passed
@kehoecj kehoecj added the OSS Community Contribution Contributions from the OSS Community label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest 🎃 Hacktoberfest 2024 hacktoberfest-accepted Valid PR Hacktoberfest PR OSS Community Contribution Contributions from the OSS Community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for setting CLI arguments using environment variables
3 participants