2.6.0 followups for release process etc. #2598
Conversation
nerrad
added
focus: documentation
|
Size Change: 0 B Total Size: 1.78 MB ℹ️ View Unchanged
|
docs/releases/readme.md
Outdated
| @@ -45,6 +45,10 @@ _Outcome_: **Team is aware of release and in agreement about what fixes & featur | |||
| - For _major_ and _minor_ releases, create branch: `release/X.X`. | |||
| - For _patch_ releases, the branch should already exist. | |||
|
|
|||
| #### Create a release pull request | |||
|
|
|||
| Using the [release pull request template](../../.github/pull_request_template.md), create a pull request for the release branch you just created. This pull request will *not* get merged to master but contains the checklist to go over as a part of the release along with being a place to contain all planning/communication around the release. The checklist should be completed and the pull request has an approved review from at least one team member before you do the Github deploy or release the plugin to WordPress.org. | |||
There was a problem hiding this comment.
For me the weirdest thing about this process was using Pull Requests for something which is not merged. Have you considered using the PR template request to merge in the testing instructions (docs/testing/releases/x) so once the PR is approved and the release happens, the PR is merged rather than closed?
There was a problem hiding this comment.
Ahh yeah that is interesting/fair point.
Have you considered using the PR template request to merge
Could do version bumps with the release I guess (still getting used to your workflow) - or is that somewhat automated?
Using something like gitflow you would have a scenario where the develop/release branch would be merged into master (or similar).
There was a problem hiding this comment.
I agree it's a bit weird, but I'm also thinking through the lense of eventual automation. Still, thinking on it, we should be merging because things like:
- {$VID} replacement for
@sincetags - Changelog
- readme.txt changes
- docs around tests etc (that you noted)
...should get merged back. I'm going to revisit this and we might need to change a bit of the process on where version replacement is done and committed so that we can merge back to master.
There was a problem hiding this comment.
So generally speaking, we do need to make sure changes get merged back to master ($:VID: tag replacement, changelog, docs updates etc) but where things get tricky is when there are cherry-picks in a release branch (which happened for both 2.6.0 and 2.6.1. So I don't think we can do an automated straight merge to master because of the conflicts from the cherry-picks (unless the automation does the merge recursively favoring a branch - which is potentially dangerous). Maybe what the automation could do is:
- attempt to do the merge of the pull request.
- if that fails then add a comment to the pull request outlining the conflicts and the release lead would have to manually merge.
But generally speaking we should favor merging pull request changes back into master so I'll update the documentation to include that.
There was a problem hiding this comment.
I just did a direct commit to master to bring in the $:VID: replacement changes done for the 2.6.0 release (see bbe27f7)
| Please include any extra details with each item as needed. | ||
| --> | ||
|
|
||
| * [ ] Have you **written tests** to cover the changes? |
There was a problem hiding this comment.
I don't see the purpose of this section in all honesty. If we're at the point of release and tests are missing a) identifying those missing tests is going to be difficult b) who is responsible for adding missing tests?
I think at this level, the checklist should ensure tests are passing.
Adding tests needs to be done prior, at issue/epic/feature level where appropriate and/or high risk.
There was a problem hiding this comment.
(General comment) My understanding is that some of the reasoning here is to form a process for consolidating the information in a central spot for easier sighting by the core wc release lead(?) - as part of feeling good about the package being updated in core.
I'm thinking of it more like part of the final checks for us to indicate that we believe we have put sufficient tests/processes in place based on our development workflow up to this point.
There was a problem hiding this comment.
As Jason pointed out, one of the purposes of this section is to communicate the state of any automated tests for the changeset in the release and if flows are not covered an opportunity to indicate what manual testing has been done (or give rationale for the confidence behind releasing if needed).
I'm thinking of it more like part of the final checks for us to indicate that we believe we have put sufficient tests/processes in place based on our development workflow up to this point.
💯 exactly.
As an example, in the release pull request for 2.6.0 I included this note with this section:
Note: Our automated test coverage is still being improved.
- The REST API Store routes have full coverage on the routes.
- There is very little automated test coverage of Cart and Checkout blocks.
- There is limited coverage of other blocks.
What this means practically is that a lot of heavy lifting is being done by manual testing for now.
With that said, I think the point you raised Mike does indicate there could be some additional areas left here for leaving comments (and maybe some pointers on these points)... I'll add.
| * [ ] mobile | ||
| * [ ] desktop | ||
| * [ ] Does this release affect API's and conform to API versioning policy? | ||
| * [ ] Have you considered the impact of the changes in the release on **other extensions** and **backward compatibility**? |
There was a problem hiding this comment.
These should be part of the release notes.
There was a problem hiding this comment.
Right, it might be good for me to add a section on followup tasks (which include release notes etc). I think some tasks might need to be written before the release, but published after the release. So I might try to nail that down a bit clearer in this via comments.
| @@ -45,6 +45,10 @@ _Outcome_: **Team is aware of release and in agreement about what fixes & featur | |||
| - For _major_ and _minor_ releases, create branch: `release/X.X`. | |||
| - For _patch_ releases, the branch should already exist. | |||
|
|
|||
| #### Create a release pull request | |||
There was a problem hiding this comment.
Can we use a github action to detect a major/minor update and create this for us?
There was a problem hiding this comment.
Yeah I have plans for that, but I want to get this merged first so we have the process in place until the action is done.
What I'd like to do is have a pull automatically created using the template when we push a specifically named branch (i.e. release/x.x) (and automatically add known details about into the pull description - version etc)
| @@ -45,6 +45,10 @@ _Outcome_: **Team is aware of release and in agreement about what fixes & featur | |||
| - For _major_ and _minor_ releases, create branch: `release/X.X`. | |||
| - For _patch_ releases, the branch should already exist. | |||
|
|
|||
| #### Create a release pull request | |||
There was a problem hiding this comment.
This should clarify which versions need this template - patches do not need this level of scuitiny.
There was a problem hiding this comment.
I disagree, I think we should be consistent in all our releases to use the same process. The checks will be more straightforward and less notes but since we are still going to be creating tags for these patches and then a pull request in WC core to bump the tag, I think copying the completed description will work a lot better for that (and I want to automate that as well).
There was a problem hiding this comment.
Thats a lot of work for a patch release which will interfere with other cycle work. Look at 2.6.1 - there was a single change there. Are you suggesting we need a throwaway PR to communicate something so small?
There was a problem hiding this comment.
Are you suggesting we need a throwaway PR to communicate something so small?
I don't think it's a throw-away PR since the notes from the PR will be copied into the pull request bumping the tag for WC core. I also disagree that it's a lot of work. More work? yes. At what point do we decide there are enough changes in a patch release to warrant additional notes?
Another important thing, from the perspective of our team, these PR's can seem redundant, but they aren't done primarily for our benefit (although I do think there is benefit for us too). Primarily this process is done for the benefit of the WooCommerce core team and the pull request we create in WooCommerce core for merging the changes. Once it becomes a part of our workflow (and hopefully automated soon), the extra time cost I think is worth it when it comes to consistent communication and process around our releases.
Viewed through the lense of the 15 patch releases we had for 2.5.14 I can certainly understand your sentiment on the cost, but viewed through the lense of moving back to bi-weekly releases, I don't anticipate we'll have that many patch releases that make this a significant chore.
There was a problem hiding this comment.
I don't think it's a throw-away PR since the notes from the PR will be copied into the pull request bumping the tag for WC core
Noting we also need to update:
- The release tag description.
- The dev blog.
- Possibly p2s.
Even if thats duplication and can be copy/pasted, if notes need updating that is a lot of places to keep in sync which is part of my concern. The other part is the large checklist in the PR template, which I get is somewhat optional, but it's still there.
At what point do we decide there are enough changes in a patch release to warrant additional notes?
If our changelog entries for patch release fixes are descriptive enough, the need for supplemental notes should be minimal fwiw. I understand supplemental content is useful for new features but that doesn't apply here.
There was a problem hiding this comment.
Your concerns are noted, but I still think having the checklist consistently applied removes a roadblock in terms of "Does this release need more things noted? Do I have everything covered? Are the appropriate places updated?" For patch releases there definitely will be cases where whoever is doing the release just checks the items and has no additional notes to add - but once this is automated, I don't think it will be that much of a chore. As I implement the automation I can look at pulling in the generated changelog directly into the pull request description for patch releases to eliminate that portion of the notes.
Even if thats duplication and can be copy/pasted, if notes need updating that is a lot of places to keep in sync which is part of my concern.
I would agree if this is something that needs kept in sync over a period of time, but for a release, that's just a one time thing right? So copy/paste should be sufficient where its needed (and copy/paste is a good signal of what can be automated as well - for instance release tag description and maybe eventually even dev blog post via WP REST API).
The purpose of the manual work initially is to help both refine what works and doesn't work, and also identify what could be automated (which I'm pretty keen on implementing asap).
While I think your concerns are important, I'd still like to try this and see how it works.
docs/releases/readme.md
Outdated
| #### Update wiki with any changes | ||
|
|
||
| - By now the testing notes for the release should have a document in `/docs/releases` along with updating the [table of contents there](../testing/releases/README.md). | ||
| - Also update the [wiki sidebar](https://github.com/woocommerce/woocommerce-gutenberg-products-block/wiki/_Sidebar/_edit) with the new links for the release testing notes. |
There was a problem hiding this comment.
Is release notes are in the codebase, why don't we include a link to the release directory on the wiki (or sidebar) and remove this extra step? Link to /releases/ and you can see the directory structure and navigate to where you want.
There was a problem hiding this comment.
Ya good point, I'll make that change. Done (updated existing wiki too)
| * [ ] What is being introduced in this release? | ||
| * [ ] If this release has potentially **high customer impact**, is a blog post (section) ready? Provide a link if applicable. | ||
| * [ ] Is the **relevant developer documentation updated**? Please provide links if applicable. | ||
| * [ ] Any special instructions or helpful notes for Happiness engineers are posted (this might not be needed for every release). |
There was a problem hiding this comment.
Just a small comment, we could redo this section to show the structure of the communication test we're after rather than having bullets. It might be easier to write that way and avoid checklist items for things not applicable.
e.g.
{version} introduces the following major features...
Developer documentation for this release covers:
- {subject}
Just a thought.
There was a problem hiding this comment.
Ya I like that idea, I'll work out something for that.
| * [ ] Does the release change the signature of any public methods or functions? | ||
| * [ ] Are filters or hooks affected by the PR code changes? | ||
| * [ ] Does the release include a changelog entry in the readme.txt? | ||
| * [ ] Please provide a link to **testing instructions** for this release. |
There was a problem hiding this comment.
O, are these merged before the PR template is made? Or where do they live? As a file or wiki or both or where? If we're testing internally only, is the repo the best place for them verses p2 or master thread? Just curious.
There was a problem hiding this comment.
Good questions (and deserving some more explanation in the doc as well).
While there will be testing that happens during development, I view this section as any extra verification testing steps for things included in the release that is done before releasing. It will be up to the release lead reviewing the changeset to decide what these testing notes include and the confidence level they have in what needs manually tested.
I think I can do some improvement on wording around this both to provide a space for where the link to testing notes are (or instructions on where they go) and the checklist just indicates they've been added (if needed).
| * [ ] Are filters or hooks affected by the PR code changes? | ||
| * [ ] Does the release include a changelog entry in the readme.txt? | ||
| * [ ] Please provide a link to **testing instructions** for this release. | ||
| * [ ] Have you reviewed impacts to performance (bundle sizes, query time etc)? |
There was a problem hiding this comment.
How are we meant to review this at this stage? It's done per PR during the cycle.
There was a problem hiding this comment.
There's two aspects to this:
- First, yes each pull has (or should have) a bundle size (performance impact) measurement. But sometimes given the development that happens over a longer period of time (or when a feature behind a gate has that gate removed for it's initial release) the impact on bundle size is needing more obvious call-out/decision. An example is 2.6.0 where we noticed that All Products and Filter blocks bundle sizes jumped significantly, but we decided to release anyways (with an eye to figuring out why the jump and improving in later iteration):
Note: As noted by @senadir here, there is some impact to bundle sizes for frontend js built for the All Products and Filter blocks. We are aware of this impact which is caused by some dependencies pulling in DashIcons and have a potential fix that is not ready for this release. The target is 2.7.0 for having this fixed so that when the feature plugin is updated in WooCommerce core this impact is not included there.
(Note: see #2280 - which I just realized wasn't prioritized that will address this, so updated)
- Second, this is another place where the release lead should review the changeset in the release and test any areas where there may be concern around performance degradation (that might not have been caught in individual pulls) or raise the flag for feedback on suspected degradations so that the team can weigh in on whether it's of concern or not and why. Yes this potentially slows down a release, but in most cases this would simply be a matter of checking the box because of our current process of doing those checks with each pull.
One thing I can do here is add a comment block for folks to add any notes about this if needed.
There was a problem hiding this comment.
In the changes I'm pushing shortly, I thought it'd be good to break this out into known negative performance impacts and known positive performance impacts (because it's just as important to highlight positive gains along with negative gains!).
Sometimes this info will be included in release posts, sometimes not, so I think it's still useful to break out into it's own section as a part of the "quality" area.
|
This is also something that could be hopefully included in the automation as well (add a section to the pull request listing all the issues/pulls that have that label for including in the release post). |
- Include about creating a release pull request and the template to use. - Include about the process for creating the package update bump in WooCommerce core.
Co-authored-by: Mike Jolley <[email protected]> Co-authored-by: Albert Juhé Lluveras <[email protected]>
I agree that there's more being added, but once the automation is added, I hope it reduces the friction. We certainly can evaluate this process once we've been through a few releases (including the package merges to core) and see if there are other improvements/adjustments that can be made. |
nerrad
force-pushed
the
docs/2.6.0-followups
branch
from
0df133d to
e17151a
Compare
- make checklist more assertive instead of inquisitive. - Provide more direction via notes and leading text in the template. - include section on devnotes.
|
Edits complete, ready for another review! |
Beginning with the
2.6.0release, we implemented a few tweaks to our release process:2.7.0because we don't have feature gating in place yet for keeping work from core we're not ready to expose there yet).This pull adds the following as a result of these tweaks and completes the follow-up todos from the 2.6.0 release post:
2.6.0release to the docs folder (and once this pull is merged, the wiki links should work for these docs).There's no impact in this pull to released code - so no testing needed.