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

docs: Attempt to flesh out the different release responsibilities #20851

Merged
merged 7 commits into from
Apr 29, 2022

Conversation

RyanTheOptimist
Copy link
Contributor

Signed-off-by: Ryan Hamilton [email protected]

docs: Attempt to flesh out the different release responsibilitiesRisk Level:
Testing: N/A
Docs Changes: RELEASES.md
Release Notes: N/A
Platform Specific Features: N/A

@RyanTheOptimist
Copy link
Contributor Author

/assign @mattklein123

@RyanTheOptimist
Copy link
Contributor Author

I have a few questions which I've noted as "TODO" in the doc. Hopefully we can sort them out :)

RELEASES.md Outdated Show resolved Hide resolved
@phlax
Copy link
Member

phlax commented Apr 16, 2022

couple of general points re release/patches

i think its important that we keep the stable branches' CI working. If we dont it makes it harder to land patches and see actual breakages etc. It also makes it harder to backport anything as every PR requires a senior maintainer to push past breakages.

~related to this point. As release managers are not maintainers they generally cant land PRs - so even with a non-broken release branch, PRs need still to be assigned/reviewed.

Im wondering if it is worth having a rotation for a "release maintainer" that shadows the release manager, and can land PRs etc

Ideally we need to also account for non-security-patch releases on supported branches - if for no other reason than so we can address eg container issues in a timely fashion.

I think it would be a good thing to make it easier to land/test/release on the stable branches, outside of sec patches, and encouraging more bug-fix backports

wrt to the first point - i think we can/should add some scheduled CI to make sure the stable branches are passing

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I left one major comment about the stability/security split, and I think @phlax also left a lot of good comments. Not all of them are actionable in this PR but we could use them to guide some of the text and future work.

/wait

RELEASES.md Outdated
Comment on lines 11 to 13
TODO(RyanTheOptimist): It seems to me that we have two kinds of releases and it would be helpful for
this doc to make a clear distinction between them. For the sake of argument, I've called them
"Major Release" (v1.22) and "Security Releases" (v1.22.1, etc). Does this terminology work?
Copy link
Member

Choose a reason for hiding this comment

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

I think we effectively have 3 (well 2 with 1 having 2 subtypes):

  1. Major release
  2. Minor release (security)
  3. Minor release (stability)

The release mechanics for 2 and 3 are very similar, there is just more private coordination required for 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, interesting. I wasn't aware that we did minor releases for non-security reasons. Are these ad-hoc releases, or are they scheduled in some way? Presumably we should talk about these releases in this doc somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

They are basically ad-hoc and best effort. It's essentially if someone wants to do the work. I agree we should discuss this option and make it clear that it's best effort and requires people to volunteer for release work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've updated this section to hopefully make this point clear. WDYT?

RELEASES.md Outdated Show resolved Hide resolved
RELEASES.md Outdated
Comment on lines 51 to 52
Major releases are handled by Matt Klein ([mattklein123](https://github.com/mattklein123))
or Alyssa Wilk ([alyssawilk](https://github.com/alyssawilk)) and do not involve any backports.
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if other people did this. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely agree :) Should we set up a rotation? I'd be happy to add a column to the schedule below.

Are there any steps in the stable release process with require a repo admin? (I seem to recall needing repo admin help from repo admins in the security release process).

Copy link
Member

Choose a reason for hiding this comment

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

Sure a rotation would be great. I think it might require an admin to actually make the release (I'm not sure) but that is a tiny step.

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 talked to @alyssawilk about this and she proposed making this the responsibility of the on-call maintainer, which makes a lot of sense to me. It's not much work in the grand scheme of things so it should fit nicely on that person's plate.

RELEASES.md Outdated Show resolved Hide resolved
Signed-off-by: Ryan Hamilton <[email protected]>
Copy link
Contributor Author

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Thanks for the great comments, both of you!

RELEASES.md Outdated
Comment on lines 11 to 13
TODO(RyanTheOptimist): It seems to me that we have two kinds of releases and it would be helpful for
this doc to make a clear distinction between them. For the sake of argument, I've called them
"Major Release" (v1.22) and "Security Releases" (v1.22.1, etc). Does this terminology work?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, interesting. I wasn't aware that we did minor releases for non-security reasons. Are these ad-hoc releases, or are they scheduled in some way? Presumably we should talk about these releases in this doc somewhere?

RELEASES.md Outdated Show resolved Hide resolved
RELEASES.md Outdated
Comment on lines 51 to 52
Major releases are handled by Matt Klein ([mattklein123](https://github.com/mattklein123))
or Alyssa Wilk ([alyssawilk](https://github.com/alyssawilk)) and do not involve any backports.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely agree :) Should we set up a rotation? I'd be happy to add a column to the schedule below.

Are there any steps in the stable release process with require a repo admin? (I seem to recall needing repo admin help from repo admins in the security release process).

RELEASES.md Outdated Show resolved Hide resolved
@phlax
Copy link
Member

phlax commented Apr 25, 2022

Another issue i want to flag is around release versioning

Historically we had a ~bitrot issue where version histories contained links to removed code that was causing docs builds to fail (and a ton of workarounds to make links point to vaguely relevant current code)

As a result, i added "intersphinx mappings" which allows us to point to specific published versions in version histories, so the links should theoretically remain correct - they are essentially permalinked

This also however, complexifies the release process for a few reasons

  • we cant link to historical docs until they have been published
  • we want the current version history - ie the current changelog - to point to the current doctree now but use version links in the future
  • the mappings in docs/conf.py need to be maintained

all in all, it means that quite specific sets of steps are required for releasing either from main or from a release branch, and that the steps have to be done in roughly the correct order.

some of this can most likely be automated, but one difficulty is automating updating ref links in the version history file as its not easily parsable/editable programatically

i will try to figure out a smoother workflow and automate what i can. For now im going to manually clean up the version histories on main and current stable branches, and i guess take responsibility for making any preparations before releases, and cleaning up after

related (#20961 #20963 #20965 )

@RyanTheOptimist
Copy link
Contributor Author

Another issue i want to flag is around release versioning

Historically we had a ~bitrot issue where version histories contained links to removed code that was causing docs builds to fail (and a ton of workarounds to make links point to vaguely relevant current code)

As a result, i added "intersphinx mappings" which allows us to point to specific published versions in version histories, so the links should theoretically remain correct - they are essentially permalinked

This also however, complexifies the release process for a few reasons

  • we cant link to historical docs until they have been published
  • we want the current version history - ie the current changelog - to point to the current doctree now but use version links in the future
  • the mappings in docs/conf.py need to be maintained

all in all, it means that quite specific sets of steps are required for releasing either from main or from a release branch, and that the steps have to be done in roughly the correct order.

some of this can most likely be automated, but one difficulty is automating updating ref links in the version history file as its not easily parsable/editable programatically

i will try to figure out a smoother workflow and automate what i can. For now im going to manually clean up the version histories on main and current stable branches, and i guess take responsibility for making any preparations before releases, and cleaning up after

related (#20961 #20963 #20965 )

Sounds great. It'd be great to write this up when you get this worked out (which I assume is already your plan :>)

@RyanTheOptimist
Copy link
Contributor Author

~related to this point. As release managers are not maintainers they generally cant land PRs - so even with a non-broken release branch, PRs need still to be assigned/reviewed.

Im wondering if it is worth having a rotation for a "release maintainer" that shadows the release manager, and can land PRs etc

I think we can probably use the on-call maintainer for this role. WDYT?

@RyanTheOptimist
Copy link
Contributor Author

/assign @alyssawilk

@phlax
Copy link
Member

phlax commented Apr 26, 2022

Release steps

Posting some notes and steps i have taken re release

Release, versions and version mappings

we always want/need the current.rst to point to the "current" docs (for any branch)

on current main 1.23 is not yet published - likewise in a release branch, the repo doesnt have the .inv file/link for mapping that minor version, so links in all of the patch version eg v1.19.x version histories should only point to the docs for that release (or lesser versions where required)

Steps required

using the release branch that i have just worked on (v1.19.4), these are ~the steps i had to take

before release (on release branch):

  • Edit current.rst - "Pending" -> "date"
  • Edit VERSION(.txt) to remove -dev

after release (on release branch):

  • Copy current.rst -> docs/root/version_history/v1.19.4.rst
  • Add 1.19.4 to TOC in docs/root/version_history/version_history.rst
  • Blank current.rst and edit - "date" -> "Pending"
  • Edit VERSION(.txt) to readd -dev and increment version

after release (on main , or possibly on any newer branches)

(to be comprehensive we may wish to do ^^ step on all newer release branches not just main)

after release (on envoy-website)

  • Edit _data/versions.yaml to include 1.19.4 (in the case of new releases/EOL etc we may wish to cycle stable/archived here at this point)

regarding editing the ref: links the only time they need to be edited is when a release has been cut (and published) and only when copying the version history files into the newer (release) branch (ie main)

almost certainly a bunch of this can be automated altho there are some issues, eg:

  • editing the rst is non-trivial programatically
  • if we dont human edit/cycle versions etc then we need some "source of authority" as to what are the current stable versions etc

@phlax
Copy link
Member

phlax commented Apr 26, 2022

re "release maintainer" mho is that it would be better working on same cycle as release for a few reasons

  • provides the release manager (who may not know expectations, or how to do things) someone to ping
  • requires more of a release-cycle focus
    • releases tend to be planned across weeks/months rather than a week
    • backporting a single patch often takes longer than a week

just my 2p tho - i think anything we can do to make it clearer/easier to contribute is a good thing

we may also want to make more use of milestones for tracking (patch) releases and related tickets/blockers/etc

Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Thanks for this clean up!

RELEASES.md Outdated Show resolved Hide resolved
responsible for approving and merging backports. The Fix Lead is a member of the security
team and is responsible for coordinating the overall release. This includes identifying
issues to be fixed in the release, communications with the Envoy community, and the
actual mechanics of the release itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to link to the docs here? is there anything private in the doc?

AFIK the release manager generally responsible for doing backports for releases.

Also somewhere maybe we should outline all the responsibilities for the role? Right now they're docced up here https://docs.google.com/document/d/1AnIqmJlGlN0nZaxDme2uMjcO9VJxIokGDMYsq2IZM98/edit but we could fold the content in here and delete that doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't realize that doc existed. I added a link to it. But I'd be happy to inline it here, if you think that would be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think inlining would be better but you're already voluntering for enough improvements so your call :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do! But I'll do it in a follow up so as not to drag this PR out any longer.

RELEASES.md Show resolved Hide resolved
* Begin marshalling the ongoing PR flow in this repo. Ask maintainers to hold off merging any
particularly risky PRs until after the release is tagged. This is because we aim for main to be
at release candidate quality at all times.
* Do a final check of the [release notes](docs/root/version_history/current.rst):
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @phlax I assume we should update links to version-specific links in this step right?
Ryan maybe link to one of @phlax recent PRs for an example?

Copy link
Member

@phlax phlax Apr 26, 2022

Choose a reason for hiding this comment

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

no - not here - this branch wants to keep links to its own version - we only update the ref: links after a release

in the case of a release from main (described as "major release" here) we want to update the ref: links for the v1.X.Y.rst files after the release has happened

this is because main is now on a new (pending) version so the just released version has now become historical - any future branches will also carry the updated ref: s

to use a concrete example - at the point of just happened v1.22.0:

  • when release happens current.rst has links with no version prefix
  • after release v1.22.0 is now historical so all links need to be updated on main
  • on the release/v1.22 branch (and patches cut from it) the links from all v1.22.x.rst files will always not have the link prefix as the version is contemporaneous

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phlax Can you suggest text to be added here?

Copy link
Member

Choose a reason for hiding this comment

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

i think the text here is fine - its below that we need to update....

@phlax
Copy link
Member

phlax commented Apr 26, 2022

re release process im experimenting with some CI which will at least flag some of the issues - like the recent one we had with a -dev release (#21022 )

as i work on it ill try and figure out any autofix/mate opportunities

Signed-off-by: Ryan Hamilton <[email protected]>
RELEASES.md Show resolved Hide resolved
Signed-off-by: Ryan Hamilton <[email protected]>
* Make sure we tweet the new release: either have Matt do it or email [email protected] and ask them to do an Envoy account
post.
* Do a new PR to setup the next version
* Update [VERSION.txt](VERSION.txt) to the next development release. E.g., "1.7.0-dev".
Copy link
Member

@phlax phlax Apr 26, 2022

Choose a reason for hiding this comment

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

* Edit `docs/conf.py` adding a version to `intersphinx_mapping`, eg:
\```python
    'v1.16': ('https://www.envoyproxy.io/docs/envoy/v1.16.0', None),

\```

this should work for now - there is another step required to sync inventories and use a local copy - but some missing railway track for that

Copy link
Member

Choose a reason for hiding this comment

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

lets not worry about this for now and i will either try to automate or add relevant docs

if we dont resolve this, it will cause issues down the road, and has mostly been forgotten, so ill try to prioritize automating this update, or making it unnecessary

RELEASES.md Show resolved Hide resolved
@phlax
Copy link
Member

phlax commented Apr 27, 2022

So these are examples of the PRs required before/after a patch release (in this case v1.20.3):

before

Close the release branch (make non-dev, fix version and update current)

#20824

after

Update version_history/inventory on main

#21035

Update website docs versions

envoyproxy/envoy-website#253

Re-open the release branch (make dev)

#21037


this is not exactly the same as a main branch release but the principles are

also this example doesnt really have any :ref: links to update so also doesnt show that v well

Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

cool LGTM with optional addition mentioned. I'll defer to phlax for the rest :-)

responsible for approving and merging backports. The Fix Lead is a member of the security
team and is responsible for coordinating the overall release. This includes identifying
issues to be fixed in the release, communications with the Envoy community, and the
actual mechanics of the release itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think inlining would be better but you're already voluntering for enough improvements so your call :-)

Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

@RyanTheOptimist thanks for working on this

mostly, im feeling lets land and iterate

would be good to remove the instructions for updating the website now i think, as these will be almost immediately obsolete

i can update/remove anything else after as we change stuff

tag build to make sure that the final docker images get pushed along with
the final docs. The final documentation will end up in the
[envoy-website repository](https://github.com/envoyproxy/envoy-website/tree/main/docs/envoy).
* Update the website ([example PR](https://github.com/envoyproxy/envoy-website/pull/148)) for the new release.
Copy link
Member

Choose a reason for hiding this comment

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

lets just remove this - its not quite correct, and is about to be obsolete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

* Make sure we tweet the new release: either have Matt do it or email [email protected] and ask them to do an Envoy account
post.
* Do a new PR to setup the next version
* Update [VERSION.txt](VERSION.txt) to the next development release. E.g., "1.7.0-dev".
Copy link
Member

Choose a reason for hiding this comment

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

lets not worry about this for now and i will either try to automate or add relevant docs

if we dont resolve this, it will cause issues down the road, and has mostly been forgotten, so ill try to prioritize automating this update, or making it unnecessary

RELEASES.md Show resolved Hide resolved
Copy link
Contributor Author

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Thanks for the review @phlax
If this work for you, I'm happy to land it and we can iterate form there...

responsible for approving and merging backports. The Fix Lead is a member of the security
team and is responsible for coordinating the overall release. This includes identifying
issues to be fixed in the release, communications with the Envoy community, and the
actual mechanics of the release itself.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do! But I'll do it in a follow up so as not to drag this PR out any longer.

tag build to make sure that the final docker images get pushed along with
the final docs. The final documentation will end up in the
[envoy-website repository](https://github.com/envoyproxy/envoy-website/tree/main/docs/envoy).
* Update the website ([example PR](https://github.com/envoyproxy/envoy-website/pull/148)) for the new release.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

RELEASES.md Show resolved Hide resolved
@RyanTheOptimist
Copy link
Contributor Author

@mattklein123 Are you good with this merging and we can continue to iterate?

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM to ship and iterate! Thank you!

@mattklein123 mattklein123 merged commit dba39ba into envoyproxy:main Apr 29, 2022
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants