-
Notifications
You must be signed in to change notification settings - Fork 64
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
Conversation
9f94a6d
to
5574803
Compare
Codecov ReportBase: 81.53% // Head: 81.53% // No change to project coverage 👍
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. |
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.
@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.
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.
See inline comment.
**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: .... |
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.
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.
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.
This is an example of the automation I mentioned above:
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 seems a reasonable approach.
Description:
Adding PR template for upgrading other package when major release happen.
Merge checklist:
frontend-platform
. This can be done by runningnpm start
and opening http://localhost:8080.module.config.js
file infrontend-build
.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
@edx/frontend-enterprise-catalog-search
,@edx/frontend-enterprise-logistration
,@edx/frontend-enterprise-utils
to use the same peer dependencyfrontend-component-header-edx
Post merge: