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

doc: update document for major release guide #393

Closed

Conversation

leangseu-edx
Copy link
Contributor

@leangseu-edx leangseu-edx commented Nov 2, 2022

Description:

Adding PR template for upgrading other package when major release happen.

Merge checklist:

  • Consider running your code modifications in the included example app within frontend-platform. This can be done by running npm start and opening http://localhost:8080.
  • Consider testing your code modifications in another local micro-frontend using local aliases configured via the module.config.js file in frontend-build.
  • Verify your commit title/body conforms to the conventional commits format (e.g., fix, feat) and is appropriate for your code change. Consider whether your code is a breaking change, and modify your commit accordingly.

For Major Release Only

Post merge:

  • After the build finishes for the merged commit, verify the new release has been pushed to NPM.

@leangseu-edx leangseu-edx force-pushed the leangseu-edx/update-doc-for-major-release branch from 9f94a6d to 5574803 Compare November 2, 2022 20:06
@codecov
Copy link

codecov bot commented Nov 2, 2022

Codecov Report

Base: 81.53% // Head: 81.53% // No change to project coverage 👍

Coverage data is based on head (5574803) compared to base (fb8dee3).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #393   +/-   ##
=======================================
  Coverage   81.53%   81.53%           
=======================================
  Files          38       38           
  Lines         926      926           
  Branches      169      169           
=======================================
  Hits          755      755           
  Misses        159      159           
  Partials       12       12           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

@leangseu-edx, thanks for this! I like the idea of listing downstream actions on breaking changes.

However, I have mixed feelings about including edx org-specifc checklist items on an openedx PR template: while we certainly don't want to break any downstream repos, to be fair we'd have to keep track of every org's fork. How about if instead a checklist item would say something like "Announce the breaking change in Slack and in the Forum"?

Would like to hear thoughts from @adamstankiewicz and @sarina, here.

.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

See inline comment.

.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
Comment on lines +12 to +24
**For Major Release Only**
- [ ] Update dependency/peer dependency repo to support the breaking change
- [ ] https://github.com/openedx/frontend-component-footer
- Example: https://github.com/openedx/frontend-component-footer/pull/241
- [ ] https://github.com/openedx/frontend-component-header
- Example: https://github.com/openedx/frontend-component-header/pull/275
- [ ] https://github.com/edx/frontend-component-footer-edx
- Example: https://github.com/edx/frontend-component-footer-edx/pull/208
- [ ] https://github.com/edx/frontend-component-header-edx
1. [ ] update `@edx/frontend-enterprise-catalog-search`, `@edx/frontend-enterprise-logistration`, `@edx/frontend-enterprise-utils` to use the same peer dependency
- Example: https://github.com/openedx/frontend-enterprise/pull/278
2. [ ] upgrade the release package to `frontend-component-header-edx`
- Example: ....
Copy link
Contributor

Choose a reason for hiding this comment

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

Had a conversation with the tCRIL team yesterday about this, and our position is this:

While tempting, having upstream (this repo) be responsible for downstream (frontend-component-*) is impractical: upstream can never know how many downstream repos there are, ultimately.

We feel the best solution here is for downstream to subscribe to upstream breaking changes instead. This subscription could be theoretically automated via dependabot/renovate or some other bot, which would make the solution even better than a checklist. What do you think?

To be clear, we think the For Major Release Only should be removed. The "Verify intended release version" item is a good check to keep.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an example of the automation I mentioned above:

openedx-unsupported/frontend-app-ecommerce#217

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems a reasonable approach.

@leangseu-edx leangseu-edx deleted the leangseu-edx/update-doc-for-major-release branch November 9, 2022 14:20
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