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

Documentation suggestions for the deploy, PR process #198

Merged
merged 5 commits into from
Mar 15, 2019

Conversation

katydecorah
Copy link
Contributor

I'd like to propose some documentation updates to the deploy process. The goal is to help make it clear for contributors what to include in their PR and how to deploy. In this PR, I propose:

  • Step-by-step deployment process to help standardize the release process.
  • Encourage consistent updating to changelog with un-released changes under a master heading.
  • Add pull request template with small checklist to help remind contributor of tasks (provide details, write tests, update changelog).

@andrewharvey I'd love to hear your feedback on these proposed updates 🙏

- [ ] Commit and push with commit message `vX.X.X`
- [ ] Create the git tag for the release with `git tag -a vX.X.X -m 'vX.X.X'`
- [ ] Push the tags with `git push --tags`
- [ ] Run `npm publish`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been bitten in the past with non-committed files containing secret keys in the same directory, which to much horror npm publish silently packaged up and included in the deploy 😱. In that regard I find it safer to use CI to publish the release, that said so long as people doing the npm publish are aware of this then it should be okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

find it safer to use CI to publish the release

Oh, smart! I'm not familiar with this, what does that look like in practice?

Copy link
Collaborator

@andrewharvey andrewharvey left a comment

Choose a reason for hiding this comment

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

This is great @katydecorah, I think this extra rigour and documentation to the process improves the quality of the project. Apart from the comments I've made, it looks good to me.

3. Request a review PR and then merge it into `master`.
4. Tag the release and start the build.
- [ ] Make sure you've pulled in all changes to `master` locally.
- [ ] Build the release with `npm run prepublish && npm run docs`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think doc changes should be committed as part of each PR in the lead up to a release, or left out to the release PR?

I think it's better to commit them as we go for each feature, that way you have complete docs for any given commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better to commit them as we go for each feature, that way you have complete docs for any given commit.

I agree, I think we should encourage PRs to update the changelog before merging to help keep track of what's in master. I included this as a step in the pull request template.

Katy DeCorah added 2 commits March 14, 2019 09:34
* master:
  prepare to publish v3.1.6
  update package-lock
  bump package version to v3.1.5
  update changelog for v3.1.5
  use indexOf instead of includes
  use XMLHttpRequest
  use sha.js instead of crypto
  use https library instead of request
@katydecorah katydecorah merged commit 95175b4 into master Mar 15, 2019
@katydecorah katydecorah deleted the update-deploy-steps branch March 15, 2019 14:24
katydecorah pushed a commit that referenced this pull request Mar 20, 2019
* version4: (25 commits)
  fix focus trap (#220)
  add `npm run docs` step to template (#221)
  Fix typo in documentation
  additional properties existance check
  Ensure properties exist prior to checking id
  update changelog
  remove use of hardcoded feature IDs
  update outdated packages with npm update
  update changelog
  Bump insecure dev dependencies
  tests for options.flyTo
  Update changelog
  update jsdoc and api docs
  pass flyTo options to map upon selection
  extend eslint, add precommit hook (#210)
  replace sha.js with nanoid reduce bundle size to 42kb (13kb gzipped)
  [docs] update deploy process (#198)
  prepare to publish v3.1.6
  update package-lock
  bump package version to v3.1.5
  ...
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