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

Update instructions in CONTRIBUTING.md #54

Merged
merged 3 commits into from
Mar 19, 2018
Merged

Conversation

seoester
Copy link
Contributor

@seoester seoester commented Feb 3, 2018

This pull request updates the "Setting up Promgen for development" section in the CONTRIBUTING file.

I added some commands initialising the required settings and also switched from virtualenv to the venv module.

@codecov-io
Copy link

codecov-io commented Feb 3, 2018

Codecov Report

Merging #54 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #54   +/-   ##
=======================================
  Coverage   47.81%   47.81%           
=======================================
  Files          72       72           
  Lines        2405     2405           
=======================================
  Hits         1150     1150           
  Misses       1255     1255

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 58d9445...a02c7e8. Read the comment docs.

Write value into DEBUG file. This is more in line with
the  module which amends the environment by
reading values from files in the .

 removes environment variables if the
corresponding file is empty
Copy link
Collaborator

@kfdm kfdm left a comment

Choose a reason for hiding this comment

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

Two minor questions but otherwise looks good! Thanks again :)

CONTRIBUTING.md Outdated
# Enable DEBUG (and development) mode
echo 1 > ~/.config/promgen/DEBUG
# Copy default settings (amend by hand after)
cp promgen/tests/examples/promgen.yml
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be copied by bootstrap already, though perhaps I could have it automatically open up in $EDITOR if you thought that would be useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! You're right, bootstrap will already have written a config file.
Do you know if there is a standard command on UNIX systems to start the default editor? I know debian provides editor which maps to nano, vim, emacs, ... but I'm not sure about other distributions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

EDITOR is the one I have seen that is most common though sometimes I have seen VISUAL. Trying to find a more definitive source but it can be difficult with things like this :)

Copy link
Contributor Author

@seoester seoester Feb 6, 2018

Choose a reason for hiding this comment

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

Alright, perhaps a comment is more feasible than dissecting UNIX standard practices ;)
On my system $EDITOR is unset when starting the shell and the editor executable references nano.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it's just a random thought I had. Feel free to keep things simple :)

CONTRIBUTING.md Outdated
#### Custom Configuration Directory

By default `promgen` uses `~/.config/promgen` as its configuration directory.
This can be changed by setting the environment variable `CONFIG_DIR` to an alternative location whenever calling `promgen ...` commands.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps I should change this to PROMGEN_CONFIG? Any thoughts on that?

 * Added note about default `admin` user
 * Removed unnecessary cp command
 * Env var `CONFIG_DIR` to `PROMGEN_CONFIG_DIR`
@kfdm
Copy link
Collaborator

kfdm commented Mar 19, 2018

Hmm, not sure why I missed reviewing this again. I'll go ahead and merge this, and then update README to point to this file to avoid the duplicate content. Thank you :)

@kfdm kfdm merged commit 4767d13 into line:master Mar 19, 2018
@seoester seoester deleted the dev-instructions branch March 30, 2018 12:54
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