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

Improving the release process #27829

Merged
merged 8 commits into from
Jan 24, 2023
Merged

Conversation

ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented Nov 22, 2022

This PR aims to automate some parts of the release process.

For RC, we have a command to run the release process:

breeze release-management start-rc-process --version 2.4.3rc1 --previous-version 2.4.2

For release, the below command runs the process:

breeze release-management start-release --release-candidate 2.4.3rc1 --previous-release <PREVIOUS RELEASE>

And there's a command for creating a version branch when releasing a minor version:

breeze release-management create-minor-branch --version-branch ${VERSION_BRANCH}

@potiuk
Copy link
Member

potiuk commented Nov 22, 2022

Comment - this is super cool that breeze gets first "big" contribution like that :)

Before the previous comment, I think (as mentioned in the other comment) it would be great if the release process is automatically tested in our CI (similarly to what we do for other release-management commands) - as this is the only way we can actually make sure the commands still work after any refactor/fix/etc.

See for example here:

run: breeze release-management prepare-provider-packages --version-suffix-for-pypi dev0

BTW. That made me realise that I lost the 'breeze release-management prepare-provider-documentation` command from CI and I need to restore it. PR for that shortly.

@potiuk
Copy link
Member

potiuk commented Nov 22, 2022

@ephraimbuddy - the "prepare-provider-documenation" step is added here #27832 - let's see if it works fine. ANSWER="yes" is passed from the job environments (see the top of the file) so it should simulate the user answering "yes" to all questions.

I think it will not work, I will have to add some other "bypass" there - becasue there is a question about the type of change as well, but I will add it when I see it (not)working. Something similar you will have to do in your PR, likely for some of the operations. i think it is entirely worthy of the effort - this way you do not have to worry during release if the tool still works or not.

@ephraimbuddy
Copy link
Contributor Author

@ephraimbuddy - the "prepare-provider-documenation" step is added here #27832 - let's see if it works fine. ANSWER="yes" is passed from the job environments (see the top of the file) so it should simulate the user answering "yes" to all questions.

I think it will not work, I will have to add some other "bypass" there - becasue there is a question about the type of change as well, but I will add it when I see it (not)working. Something similar you will have to do in your PR, likely for some of the operations. i think it is entirely worthy of the effort - this way you do not have to worry during release if the tool still works or not.

In my case, I wouldn't want to run some commands if the environment is CI, so I'm looking at sorting the commands that would run if --answer yes is passed. Or just check if we are on CI and use dry run

@potiuk
Copy link
Member

potiuk commented Nov 22, 2022

@ephraimbuddy - the "prepare-provider-documenation" step is added here #27832 - let's see if it works fine. ANSWER="yes" is passed from the job environments (see the top of the file) so it should simulate the user answering "yes" to all questions.
I think it will not work, I will have to add some other "bypass" there - becasue there is a question about the type of change as well, but I will add it when I see it (not)working. Something similar you will have to do in your PR, likely for some of the operations. i think it is entirely worthy of the effort - this way you do not have to worry during release if the tool still works or not.

In my case, I wouldn't want to run some commands if the environment is CI, so I'm looking at sorting the commands that would run if --answer yes is passed. Or just check if we are on CI and use dry run

Yep. Checking for CI is a good idea.

@potiuk
Copy link
Member

potiuk commented Nov 22, 2022

Already in couple of places:

    if os.environ.get("CI"):

@ephraimbuddy
Copy link
Contributor Author

@potiuk, Concerning running this on the CI with dry_option and --answer yes, I think we should not. It looks risky and doesn't seem to give us information on whether the command is still ok. Like it just lists the commands that it should run without running them. I don't think it's useful considering that if there's any mistake on the dry_option or CI, we risk running the commands for real. The risk seems to outweigh the testing on CI

The thought of a mistake makes me want to exclude it from what we should test on the CI.

@potiuk
Copy link
Member

potiuk commented Nov 22, 2022

@potiuk, Concerning running this on the CI with dry_option and --answer yes, I think we should not. It looks risky and doesn't seem to give us information on whether the command is still ok. Like it just lists the commands that it should run without running them. I don't think it's useful considering that if there's any mistake on the dry_option or CI, we risk running the commands for real. The risk seems to outweigh the testing on CI

The thought of a mistake makes me want to exclude it from what we should test on the CI.

Setting --dry-run for all makes no sense indeed. But I think it makes perfect to do all the steps except pushing the changes (this can be skipped if CI). There is no risk involved. The tokens we have on CI in regular job are read-only so there is no risk we will actually persist any changes. The local repo is checked out locally and wiped out after the job is finished so any changes to it are not persisted - from what I see just "pushing" any changes will have to be "dry-runed" based on CI, all the other steps can be safely executed (and if you try to push something or otherwise change the state of repo - you will see a failure because the CI job has no permissions to change anything other than in local copy of the workspace.

Those are the defauilt permissions in the ci.yml:

permissions:
  # All other permissions are set to none
  contents: read
  packages: read

And in order for the job to have permission to change anything, it has to be given those permissions (this is for packages but contents: write needs to be set to be able to push to the repo:

 build-ci-images:
    permissions:
      packages: write

@ephraimbuddy
Copy link
Contributor Author

Nice. Thanks once more!

@ephraimbuddy ephraimbuddy force-pushed the automate_release branch 8 times, most recently from e13ccc9 to 0767279 Compare November 25, 2022 07:28
@ephraimbuddy ephraimbuddy force-pushed the automate_release branch 2 times, most recently from c3dd3f2 to 9833431 Compare January 22, 2023 11:40
@ephraimbuddy
Copy link
Contributor Author

I think you might wan to re-install breeze (via the composite action we use elsewhere) just before "Fix ownership" job. This should fix the last problem @ephraimbuddy

Re-installing breeze didn't work. I will come back to debug it again, I believe it's from what now runs within the code in the CI unlike before when I skipped them.

This PR aims to automate some parts of the release process.

For RC, we have a command to run the release process:

  breeze release-management start-rc-process --version 2.4.3rc1 --previous-version 2.4.2

For release, the below command runs the process:

  breeze release-management start-release --release-candidate 2.4.3rc1 --previous-release <PREVIOUS RELEASE>

And there's a command for creating a version branch when releasing a minor version:

  breeze release-management create-minor-branch --version-branch ${VERSION_BRANCH}

Use breeze's user_confirm & get_console.print

Skip some steps in CI

Run command in CI

add user_confirm_bool function, a version of user_confirm

Remove retagging of images

simplify console.print calls

Update readme config hash

Use dry-run for commands that shouldn't run in CI

Remove outdated instruction

Remove single quotes on double quote
@ephraimbuddy
Copy link
Contributor Author

Previously, some of the commands used to check out branches were being executed in the CI. However, because this change was not in the checked-out branch, the codes could not be found and the process failed. To resolve this issue, the solution was to run the branch check-out commands in dry-run mode.

@potiuk
Copy link
Member

potiuk commented Jan 23, 2023

Nice!

@potiuk
Copy link
Member

potiuk commented Jan 23, 2023

Feel free to merge if you think it's GOOD and useful :).

@ephraimbuddy
Copy link
Contributor Author

Feel free to merge if you think it's GOOD and useful :).

Been thinking about the useful side of it :) let @jedcunningham decide

@potiuk
Copy link
Member

potiuk commented Jan 23, 2023

Feel free to merge if you think it's GOOD and useful :).

Been thinking about the useful side of it :) let @jedcunningham decide

Comment here - for @jedcunningham as well. But I wonder what others involved in the process think:

I personally think it is super-useful. It makes the process repeatable but also it is largely self documenting. It's far easier to reason about it than README docs. But it does have some potential traps.

Cons:

  • it's a little magical. release manager will not have to know what's under-the hood and if there is a consistent problem, all might seem to work but the release might be "wrong"

  • when things do not work, the release manager might easily say "it does not work" and rather than fix the problem, defer it to whoever can modify breeze (and knows how it work - at least a bit)

Pros:

  • One of the useful parts of it is that it is going to be continuously tested (vast majority of it ) , so that you are not surprised by it not working any more due to unrelated changes

  • With breeze's built-in dry-run functionality, it is actually self-explanatory and if it does not work it allows the release manager to follow the whole process manually. Even if it is a little magical, you can find out how it works by --dry-run and still try to fix it manually

My personal view:

I personally usually err on the side of automation for those kind of things (as you are probably all well aware of that). One of the "features" of the Breeze rewrite in Python was to make it a little less magical - so that generally everyone here might understand and fix things. The fact that @ephraimbuddy wrote it largely himself with a little of my help is kinda proof that it works. Maybe it took a bit long and maybe there were a few things that require a bit more knowledge of the "magic" under-the-hood (especially when it comes to CI integration), but it was not that bad I think.

I think there is a big benefit of automating - it makes it actually possible to speed up cadence of releases. This is what we also see wiht @eladkal releasing the providers in increased cadence now - I think it would not be possible if not the automation of the process. There are still things there that could be improved and automated more. And "Release early, release often" is the mantra that I will continuously repeat as the way how we can improve things. The "provider" release process (after Elad's few improvements/teething problems when he started doing it) allows to run it as often as we want by anyone. We can share the burden of relases among many people, because the mundane, easy to make mistake parts of the process will be automated and only the "interesting" part remains (reviewing the changes, cherry-picking them, communicating with people about testing etc. etc. I find those parts really important and interesting and there are things I would not even want to automate (rather to make it assisted by automation).

This change above I think automates what's not "interesting" thus potentially opening the release process for more participants.

Those my 3 cents.

@jedcunningham
Copy link
Member

I'm not too concerned with it being "magic", I think we all can debug and carry on manually if things go sideways. Hopefully it doesn't, but historically we have improvements/fixes after most releases :)

I'm not sure we should use this to widen our release manager pool or speed up cadence though. The challenging part isn't this part after all. It will remove some of the tedium though, which is always good.

@ephraimbuddy ephraimbuddy merged commit 9411e39 into apache:main Jan 24, 2023
@ephraimbuddy ephraimbuddy deleted the automate_release branch January 24, 2023 08:21
@ephraimbuddy ephraimbuddy added changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) labels Jan 25, 2023
@ephraimbuddy ephraimbuddy modified the milestone: Airflow 2.5.2 Jan 25, 2023
pierrejeambrun pushed a commit that referenced this pull request Mar 6, 2023
* Improving the release process

This PR aims to automate some parts of the release process.

For RC, we have a command to run the release process:

  breeze release-management start-rc-process --version 2.4.3rc1 --previous-version 2.4.2

For release, the below command runs the process:

  breeze release-management start-release --release-candidate 2.4.3rc1 --previous-release <PREVIOUS RELEASE>

And there's a command for creating a version branch when releasing a minor version:

  breeze release-management create-minor-branch --version-branch ${VERSION_BRANCH}

Use breeze's user_confirm & get_console.print

Skip some steps in CI

Run command in CI

add user_confirm_bool function, a version of user_confirm

Remove retagging of images

simplify console.print calls

Update readme config hash

Use dry-run for commands that shouldn't run in CI

Remove outdated instruction

Remove single quotes on double quote

* fixup! Improving the release process

* fixup! fixup! Improving the release process

* remove hidden arg from commands

* Add check=True to all the run_command to raise when there's a failure

* fixup! Add check=True to all the run_command to raise when there's a failure

* Update minor release command

* Update the ci

(cherry picked from commit 9411e39)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants