-
Notifications
You must be signed in to change notification settings - Fork 0
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
doc: add contributing guide template #2
base: main
Are you sure you want to change the base?
Conversation
|
||
# Test your changes before requesting a review. | ||
# All changes should be tested. No failing tests are allowed. | ||
[TEST COMMAND] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i'd separate tests from development.
As it can be a very messy part so i'd make it separated on his own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea since some repos require a specialized setup. Packages usually have very small requirements (just run npm run test
), while services can have large requirements (run a server + configure .env + run the script).
|
||
<!-- Describe how to deploy this service to the staging environment. --> | ||
|
||
#### Production |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we specify that
- only the main branch of services is to be deployed
- it's people's responsibility to make sure their PR doesn't break production or other services (by running CI, creating tests, ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if a rollback strategy make sense to add here but. could be a good addition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we specify that only the main branch of services is to be deployed
I think this info is specific to each service, so we can probably leave it out of the template. It can be mentioned in the "Production" section.
Should we specify that it's people's responsibility to make sure their PR doesn't break production or other services (by running CI, creating tests, ...)
Yes, that is a good addition, similar to the "no failing tests are allowed" line in the "Develop" section. It could be added as a Caution block. immediately after the "Production" heading.
I don't know if a rollback strategy make sense to add here but. could be a good addition
Makes sense to me as part of the Production section.
This guide should be short and concise while providing enough information to get started. The last thing someone wants to do when they start working is read a bunch of documentation. 😅 Do you have an idea of how we could do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea a lot! I left a few comments below and replied to @Heyrows's thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONTRIBUTING.md
to follow this convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 #idea: We have a slightly different version for public packages, like @prismicio/client
. Note that we replaced mentions of specific Prismic teams with just "Prismic."
What if we had both templates available in this repo?
It would mean my comment above about renaming the file to CONTRIBUTING.md
is irrelevant. We could use this naming convention instead:
CONTRIBUTING-private.md
CONTRIBUTING-public.md
# Checkout the master branch and pull the latest changes. | ||
git checkout master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 #idea: Repositories use main
as the default branch now. Maybe we can use main
instead of master
in the template.
Resolves: N/A
Description
md
templates to this repository is the way to go to share "code" related template.Checklist
How to QA 1
Footnotes
Please use these labels when submitting a review:
⚠️ #issue: Strongly suggest a change.
❓ #ask: Ask a question.
💡 #idea: Suggest an idea.
🎉 #nice: Share a compliment. ↩