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

add support for specifying course file via environment variable #536

Merged
merged 10 commits into from
Mar 10, 2022

Conversation

intrand
Copy link
Contributor

@intrand intrand commented Mar 3, 2022

This PR fixes #503

Description

What's the goal of this PR?

To be able to configure the path to course.yml via environment variable. This avoids passing a path on the command line at every invocation, which will help with automation and possibly usability in single-cluster environments.

In addition, I've added a default path of course.yml to the courseFile. This should further help reduce typing if you're in the same directory as a course.yml (assuming it's named that).

What changes did you make?

cmd/root.go now handles course file setting precedence. This should be global to all instances of setting courseFile, not just any particular subcommand (eg, it applies to plot, convert, template, etc).

What alternative solution should we consider, if any?

Using viper to handle configuration overrides at various levels. However, that's a much larger change.

@intrand intrand added the go Pull requests that update Go code label Mar 3, 2022
@intrand intrand requested a review from sudermanjr as a code owner March 3, 2022 20:41
@intrand intrand self-assigned this Mar 3, 2022
@intrand intrand requested a review from lucasreed as a code owner March 3, 2022 20:41
@CLAassistant
Copy link

CLAassistant commented Mar 3, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@lucasreed lucasreed left a comment

Choose a reason for hiding this comment

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

It strikes me as I read this that validating the args probably never belonged in the reckoner package. Since we are, as the name implies, validating command line arguments, I think that all belongs in the cmd package. That would probably make testing easier and decrease the size of the very large reckoner package since this code doesn't really make sense there.

Once moved into the cmd package they don't need to be capitalized (exported) since they can be private functions at that point.

I have a couple other comments in-line that are less critical.

cmd/root.go Outdated Show resolved Hide resolved
pkg/reckoner/validation.go Outdated Show resolved Hide resolved
pkg/reckoner/validation_test.go Outdated Show resolved Hide resolved
@lucasreed lucasreed self-requested a review March 7, 2022 18:22
Copy link
Contributor

@lucasreed lucasreed left a comment

Choose a reason for hiding this comment

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

One small fix in klog version so we aren't re-introducing the old version. Other than that it all looks great, thanks!

cmd/validation.go Outdated Show resolved Hide resolved
intrand and others added 2 commits March 9, 2022 10:56
Co-authored-by: Luke Reed <luke@lreed.net>
@intrand
Copy link
Contributor Author

intrand commented Mar 9, 2022

All set!

@intrand intrand requested a review from lucasreed March 9, 2022 16:58
Copy link
Contributor

@lucasreed lucasreed left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! 🎉

@sonarcloud
Copy link

sonarcloud bot commented Mar 10, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@lucasreed lucasreed merged commit 785c764 into master Mar 10, 2022
@lucasreed lucasreed deleted the intrand/env branch March 10, 2022 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow reckoner to be configured via environment variables
3 participants