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

ci: support releases with branch-protection #1458

Merged
merged 10 commits into from
Dec 27, 2023

Conversation

v1v
Copy link
Member

@v1v v1v commented Nov 23, 2023

What

Split the release workflow in a different set of actions:

  1. Run a pre-release workflow
  2. Then a GitHub Pull Request will be created with the versioning changes and slack notifiaction will be sent with the PR details.
  3. When the PR is merged then
  4. Run the release workflow.

Why

There is a hard requirement to review every single change through a Pull Request. Hence releases cannot commit directly to the repository.

Implementation details

I split https://github.com/lerna/lerna/tree/main/libs/commands/publish#readme in two steps:

Rationale

Rather than automating the release step when the PR is merged, I decided to keep it simple and have two workflows that need to be triggered manually.

We can think about automating the execution of the second one when the PR is merged... but I didn't want to get to creative for the shake of simplicity.

Test

https://github.com/v1v/test-2fa

@v1v v1v requested review from a team November 24, 2023 11:06
@v1v v1v self-assigned this Nov 24, 2023
@v1v v1v marked this pull request as ready for review November 24, 2023 11:06
run: npm run ci:release

- name: Setup credentials
Copy link
Member Author

Choose a reason for hiding this comment

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

See ternary operation in the top-level env section

scripts/ci-release.mjs Outdated Show resolved Hide resolved
scripts/ci-release.mjs Outdated Show resolved Hide resolved
@devcorpio
Copy link
Contributor

devcorpio commented Nov 27, 2023

I agree, I don't see any problem with having 2 manual steps (pre-release, and release). We can revisit this in the future if we believe that automating the second step is essential.

So, If I understand, the flow will be something like this:

  • prerelease step will create a PR where we will see the bumped version of every package + changelog, etc
  • release step will create the tags and associate them to the latest commit of main (the merge commit of the PR) and will publish the packages, and will upload artifacts to the CDN.

Is this correct?

Edit: I was checking the code, and I believe the tags will be created in the PR. The tags will need to be associated to the latest commit in main instead (which will be the merge commit of the PR).

I'm saying this, because I saw the "lerna version" (creating the tags) in the pre-release, rather than creating them in the release.mjs file.

@v1v
Copy link
Member Author

v1v commented Nov 28, 2023

Edit: I was checking the code, and I believe the tags will be created in the PR. The tags will need to be associated to the latest commit in main instead (which will be the merge commit of the PR).

Good point. When I tested this out I merged with rebase and things were ok:

But I'm now a bit confused whether that behaviour might be different if using squash and merge.

Let me explore whether I can change this to use lerna version without tagging and lerna publish with tagging.

@v1v
Copy link
Member Author

v1v commented Dec 12, 2023

Let me explore whether I can change this to use lerna version without tagging and lerna publish with tagging.

cdc989e is the one to avoid tagging during the pre-release workflow. The post-release workflow then will be the one pushing the changes and creating the release.

For example, in my playground, I ran:

There is one thing that I could not figure out and was the one regarding tagging the package named https://github.com/v1v/test-2fa/tree/master/packages/test-2fa-foo for some reason it only tagged the main package called https://github.com/v1v/test-2fa/tree/master/packages/test-2fa-bar

I don't know whether tagging all the packages is a hard requirement.

@devcorpio , I need your thoughts

@vigneshshanmugam
Copy link
Member

for some reason it only tagged the main package called https://github.com/v1v/test-2fa/tree/master/packages/test-2fa-bar

Does both of the package had changes? Can you point us to the commit where this happened? Thanks.

@v1v
Copy link
Member Author

v1v commented Dec 13, 2023

Does both of the packages had changes? Can you point us to the commit where this happened? Thanks.

No code changes in any of those packages. Aha, that might be the reason.

I've just tried some code changes with v1v/test-2fa#61

Then I ran the prepare-release produced v1v/test-2fa#62
And after merging it I ran the release, so [email protected] was created but no tag/releases for the package test-2fa-foo.

IIUC, lerna version is the one producing all the relevant tags but when using --no-push no tags are created at all. And lerna publish does not know anything about those tags, that's managed by lerna version.


Finally, I'll need to explore this workaround, somehow lerna pubish does publish a new version, which it's incorrect

image

while I'd have expected the same versions as defined in v1v/test-2fa#62

I'll transition this PR to a draft PR.

@v1v v1v marked this pull request as draft December 13, 2023 07:53
@v1v
Copy link
Member Author

v1v commented Dec 13, 2023

Finally, I'll need to explore more this workaround, somehow lerna pubish does publish a new version, which it's incorrect

I followed the steps explained in lerna/lerna#1957 (comment) and it worked like a charm:

v1v/test-2fa#65 was created and when merged then the lerna publish kept those package versions:

Found 2 packages to publish:
 - test-2fa-bar => 2.1.3
 - test-2fa-foo => 3.4.0

And publish them in npmjs:

image

@v1v v1v marked this pull request as ready for review December 13, 2023 08:43
@devcorpio
Copy link
Contributor

devcorpio commented Dec 13, 2023

apologies for the delay, this afternoon/evening I will add the results of a few tests I'm doing with the current (the "old" one) Lerna setup

But from what I see of the new process, everything looks awesome.

@devcorpio
Copy link
Contributor

devcorpio commented Dec 13, 2023

apologies for the delay, this afternoon/evening I will add the results of a few tests I'm doing with the current (the "old" one) Lerna setup

@v1v

1. Changing a package which no other package depends on (e.g. apm-rum-react)

Result: Lerna only versions the mentioned package.

Screenshot 2023-12-13 at 15 49 35

2. Changing a package which other packages depend on (e.g. apm-rum-core)

Screenshot 2023-12-13 at 15 51 32

Result: Lerna versions the package I modified and the ones that depend on the modified package.

Conclusion:

The behaviour you see in the tests you are doing is correct. 🚀 One last test that you might do (if you have time) is to modify one of your packages to depend on another one, and then see if the behaviour that I just described (case 2) is reproduced, too

@vigneshshanmugam
Copy link
Member

Great work @v1v 🎉

One more thing I would like to check is whether our publishing for single package works after this change?

"release-package": "node scripts/publish-package",

Its not a huge deal if we cant make it work and not a requirement unless we need a real emergency situation. But just want to bring it to your notice.

@v1v
Copy link
Member Author

v1v commented Dec 13, 2023

One more thing I would like to check is whether our publishing for single package works after this change?

Pushing directly to the main branch is not allowed; therefore the script will not work with the default lerna.json.

@v1v
Copy link
Member Author

v1v commented Dec 18, 2023

One last test that you might do (if you have time) is to modify one of your packages to depend on another one and then see if the behaviour that I just described (case 2) is reproduced, too

I'll test this out and update this comment with the outcome.

Given the new package and its dependency; see https://github.com/v1v/test-2fa/pull/66
When I run the pre-release workflow; see https://github.com/v1v/test-2fa/actions/runs/7245859854
Then https://github.com/v1v/test-2fa/pull/67 is created

test-2fa-bar@2.1.4
test-2fa-dependent@1.0.1
test-2fa-foo@3.5.0
Given https://github.com/v1v/test-2fa/pull/67
When it's merged
And I run the release workflow; see https://github.com/v1v/test-2fa/actions/runs/7245876059
Then, the artifacts are published

Found 3 packages to publish:
 - test-2fa-bar => 2.1.4
 - test-2fa-dependent => 1.0.1
 - test-2fa-foo => 3.5.0

Copy link
Contributor

@devcorpio devcorpio left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

this week (perhaps the next one) we will do our first release with this system. We want to release the changes of this PR (once it's approved & merged)

@v1v
Copy link
Member Author

v1v commented Dec 20, 2023

this week (perhaps the next one) we will do our first release with this system. We want to release the changes of #1462 PR (once it's approved & merged)

Awesome! ❤️

I'll be on Christmas break for two weeks starting early next week. Feel free to merge this PR and run the first release with it, if something goes terribly wrong @amannocci is around, so feel free to reach him. Or, if needed, please feel free to revert this PR or wait until I'm back, whatever it suits you.

Other than that, have a lovely 🎄 time 🙇

@devcorpio
Copy link
Contributor

Awesome! ❤️

I'll be on Christmas break for two weeks starting early next week. Feel free to merge this PR and run the first release with it, if something goes terribly wrong @amannocci is around, so feel free to reach him. Or, if needed, please feel free to revert this PR or wait until I'm back, whatever it suits you.

Other than that, have a lovely 🎄 time 🙇

thank you! Have a lovely 🎄 you too 🎉

@devcorpio devcorpio force-pushed the feature/release-with-branch-protection branch from a338743 to ee3eb33 Compare December 27, 2023 10:48
@devcorpio devcorpio merged commit 1c4c0ef into elastic:main Dec 27, 2023
20 of 21 checks passed
@devcorpio
Copy link
Contributor

devcorpio commented Dec 27, 2023

@v1v

excellent! The release process worked! 🎉 🥳

--
At the same time, I found two things that we should take into account:

Example:

Screenshot tag example
  • The pre-release action doesn't work well with dry-run mode (not a big deal, since we can see how the versions will be once the PR is created, but I just wanted to share).

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.

3 participants