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

Refactored Gaia parameters #198

Merged

Conversation

michelvocks
Copy link
Member

This PR refactors Gaia's configuration parameters which had an inconsistent naming scheme and missed some important documentation.

@michelvocks michelvocks requested a review from Skarlso July 18, 2019 07:23
@@ -307,7 +306,7 @@ RUN chmod +x ./gaia-linux-amd64 \
&& chmod 600 /root/.ssh

# Set homepath as volume
VOLUME [ "${GAIA_HOMEPATH}" ]
VOLUME [ "${GAIA_HOME-PATH}" ]
Copy link
Member

Choose a reason for hiding this comment

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

- in environment properties is not a good thing as they usually can't be parsed nicely or cause problems on the command line in ZSH they even have to be explicitly escaped sometimes like [ or {. I would just leave this out. It's fine like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need some way to separate longer property options. My first idea was to use _ but if you pass parameters to the binary it also looks odd: -home_path

Copy link
Member

Choose a reason for hiding this comment

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

Oh hang on that stuff is auto-read. ARGH. Okay, never mind then.
Also, yeah it should be --home-path

Copy link
Member Author

Choose a reason for hiding this comment

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

I found out that flag supports that out of the box by replacing - with _ for environment variables. Perfect 😄

Copy link
Member

Choose a reason for hiding this comment

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

Oh very awesome! :D :D Perfect. :)

@codecov-io
Copy link

codecov-io commented Jul 18, 2019

Codecov Report

Merging #198 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #198      +/-   ##
=========================================
- Coverage   62.22%   62.2%   -0.03%     
=========================================
  Files          48      48              
  Lines        3934    3934              
=========================================
- Hits         2448    2447       -1     
- Misses       1081    1082       +1     
  Partials      405     405
Impacted Files Coverage Δ
workers/scheduler/scheduler.go 65.43% <0%> (-0.31%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2382c79...aa5c983. Read the comment docs.

Copy link
Member

@Skarlso Skarlso left a comment

Choose a reason for hiding this comment

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

Nice. I love consistency. Approved with a comment to leave the environment property as is, I think. :)

@michelvocks michelvocks merged commit fe1bdc9 into gaia-pipeline:master Jul 18, 2019
@michelvocks michelvocks deleted the configuration_param_refactor branch July 18, 2019 08:07
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.

3 participants