Skip to content

Add contributing dir with Config file checklist #7017

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

Merged
merged 2 commits into from
Jan 14, 2020
Merged

Conversation

banks
Copy link
Member

@banks banks commented Jan 9, 2020

This checklist was reviewed internally in a document form a few months ago but I wanted to make it public.

I've followed Nomad's conventions of adding a contributing dir. The only quirk here is we already have a .github/CONTRIBUTING.md which has some overlap. GitHub links to that in new PRs so it's useful to surface there and will only pick it up from .github in the root or in /docs/ dir. I don't want to add /docs and choose not to move it for now as we don't want other checklists in the root so it still doesn't end up in the same place but open to other options here.

I tried to make this a relatively minimal change and we can certainly iterate on the README and other things.

The contributing/README.md is based heavily on the Nomad one: https://github.com/hashicorp/nomad/tree/master/contributing for now. I don't know that they need to be in perfect harmony so we can iterate but it seemed like a decent starting point.

Finally I moved INTERNALS.md to avoid having a sprawl of things in different places I left a stub behind so links don't break in case anyone links to it in mailing list issues etc.

@banks banks requested a review from a team January 9, 2020 13:13
Copy link
Contributor

@crhino crhino left a comment

Choose a reason for hiding this comment

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

Overall, I like this idea a lot. I think it will help distribute knowledge and enable better contributions.

I especially thought the explanations of why/when you would need to go through each of the four cases presented was useful to know whether I needed to do it or not.

I had to do the first 2 steps recently with hclog, so I mimicked running through this checklist with that change. I found that using a gist was not the greatest UX, since it only showed me the raw markdown. I ended up copying it again in a comment below (https://gist.github.com/crhino/6cefaa4a1f06aebbcc3ef274f3b06b58). Not sure whether to keep that suggestion in or not.

I also liked having the exact file locations pointed out, and I added a few comments updating some of the bullet points with full paths.

@banks
Copy link
Member Author

banks commented Jan 10, 2020

Great feedback @crhino thanks!

I found that using a gist was not the greatest UX, since it only showed me the raw markdown.

If you name the "file" something that ends in *.md then gists will format it as Markdown and include clickable check boxes: https://gist.github.com/banks/35e4af7feb4c8f0db7a8a645de29bde9 (you probably need to fork that to make the checkboxes clickable as it edits the underlying markdown).

Do you think it would help to be more explicit about that in the README?

@crhino
Copy link
Contributor

crhino commented Jan 10, 2020

Ah, good to know that gists can do that! I don't think it is necessary to go into that much detail in our README, the essential point is to include the checklist with the PR in my opinion, the workflow used to get there doesn't have to be too detailed.

Copy link
Contributor

@crhino crhino left a comment

Choose a reason for hiding this comment

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

LGTM

@banks banks merged commit 86692f9 into master Jan 14, 2020
@banks banks deleted the config-checklist branch January 14, 2020 12:24
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.

2 participants