fix(release): allow multiple tags on the same commit for separate branches #2621

abdsahin · 2023-04-19T07:41:17Z

Example use case: after testing a pre-release, if all good with the pre-release, it should be promotable to the next stage with the same commit.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…lease branches

mrgrain · 2023-04-19T13:40:49Z

Hi @abdsahin Can you confirm the scenario with you. Do I understand this correctly, with this PR merged, if the release workflow (or task) is run multiple times in a row a new version is released everytime?

I'm also a bit confused how one would achieve the change without a new commit, since you have to add/remove/modify the prerelease property.

abdsahin · 2023-04-19T17:11:47Z

Following is from the content of the .projenrc.ts

defaultReleaseBranch: "main",
prerelease: "pre", //main branch is designed to be a pre-release of the major release
releaseBranches: {
  "release/0.x": {
    majorVersion: 0,
  },
},

The use case is our developers are creating features branches and merges to main. Ones all tests are done, we create another merge request to release/0.x.

Now, assume the merge strategy is fast-forward without "squash commit" ,so no merge commits are created. release/0.x and main has the same commits.

I am trying to create a re-usable library, cannot be certain which teams will use merge commits and which not, therefore I thought this might be an improvement, but happy to hear your feedback.

mrgrain · 2023-04-19T20:07:30Z

Thanks for the explanation @abdsahin ! This is very cool usage of the release branch feature. It didn't occur to me yet that you could do it that way! 🤯

I think my main concern here is that current users might be working on the assumptions that releases are idempotent. By that I mean that running the release task/workflow on the same commit they will get exactly one version.

It seems (but I'm not 100% sure) that this PR changes this behavior. So I'd like to understand better if that's a desirable change to the default behavior. For example if the re-releasing can only happen under very specific circumstances, we might be okay merging it as a default. If not, we will need an option for it.

If any default behavior is changed, we should run campaign before applying it.

abdsahin · 2023-04-19T21:07:52Z

For the above configuration I mentioned, if there is a v1.0.0-pre.0 release on main branch and if you apply the same commit on release/0.x branch, you try to release v1.0.0-pre.0 again on the wrong branch.

The PR changes only this behaviour by strictly checking the current branch.

Probably everyone is using merge commits and don't see that issue, so I can enforce using merge commits as well.

abdsahin · 2023-04-20T15:10:29Z

@mrgrain , how about adding a new property named branchIdenpotentRelease( I am open to suggestions for the name), set it as false by default and if true then we can activate the changes in this PR?

I can update the PR accordingly.

mrgrain

Yeah I think a property would definitely be the safest option. But tbh I'm still trying to understand how this change is working. It might be okay to be released as a default.

mrgrain · 2023-04-20T15:46:54Z

src/release/bump-version.ts

+ const latestTagPrerelease = getPreRelease(latestTag);
+ // returns true if the latest tag on this commit is on the same release branch
+ const latestTagSamePrerelease =
+ (prerelease &&
+ latestTagPrerelease &&
+ latestTagPrerelease[0] === prerelease) ||
+ (!prerelease && !latestTagPrerelease);


I have a hard time understanding what this check is semantically doing.

Could you try and refactor this somehow? Maybe using helper methods that are descriptively named.

I tried to explain it a bit better.

codecov-commenter · 2023-04-20T21:03:40Z

Codecov Report

Merging #2621 (52a65f9) into main (0825edd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##             main    #2621   +/-   ##
=======================================
  Coverage   96.30%   96.31%           
=======================================
  Files         181      181           
  Lines       33385    33401   +16     
  Branches     3061     3066    +5     
=======================================
+ Hits        32153    32171   +18     
+ Misses       1232     1230    -2

Impacted Files	Coverage Δ
src/release/bump-version.ts	`99.44% <100.00%> (+0.02%)`	⬆️

... and 1 file with indirect coverage changes

abdsahin · 2023-04-23T13:06:06Z

Yeah I think a property would definitely be the safest option. But tbh I'm still trying to understand how this change is working. It might be okay to be released as a default.

@mrgrain, is it clear now? Do you still think we need a property to enable that behavior?

mrgrain · 2023-04-25T08:46:47Z

src/release/bump-version.ts

+ (!prerelease && !latestTagPrerelease); // If we are bumping a regular release( example: 1.0.5) now and also latest release was a regular release
+
+ /**
+ * Skip the bump only if the current commit is already tagged within the same branch


With all the previous assertions, I don't follow how we can make the conclusion that the current commit is tagged on the same branch. Can you elaborate on this please?

Each branch is mapped to a specific prerelease component.

releaseBranches:{ "release/1.x.alpha": { majorVersion: 1, prerelease: "alpha" }, "release/1.x.beta": { majorVersion: 1, prerelease: "beta" }, "release/1.x.rc": { majorVersion: 1, prerelease: "rc" }, "release/1.x": { majorVersion: 1 } }

So now

If it is a regular release and getPrereleaseComponent(latestTag) returns null meaning latest release was also a regular release.

If it is a prerelease, we compare the current commit prerelease component with the latest release one. As suggested I will try to add that into a helper function.

mrgrain

Okay thanks for the explanations so far. I still have one question of understanding, but I'm sure you'll be able to clear that up for me.

We will need a new options for this that is false by default. Something along the lines of allowMultipleReleasesOfSameCommit and makes sure the docstring explains this scenario in detail.

src/release/bump-version.ts

mrgrain · 2023-04-25T08:52:12Z

src/release/bump-version.ts

- if (releaseTags && compare(releaseTags[0], prereleaseTags[0]) === 1) {
+ if (
+ releaseTags &&
+ releaseTags.length > 0 &&


Is this fixing a bug or something?

If it is required, let's also pull this out into a helper function.

Yes this is for bug fixing, there are cases where releaseTags is returned as empty array and going directly into the if case rather than else.

mrgrain · 2023-04-25T09:02:42Z

Alternatively taking a step back @abdsahin - Maybe what we actually want here is a --force option on the command that users can use to always force a new bump. You could then implement additional checks in userland.

I think I would very much prefer this alternative option. Do you think this would allow you to solve your problem in userland?

abdsahin · 2023-04-26T10:16:11Z

Alternatively taking a step back @abdsahin - Maybe what we actually want here is a --force option on the command that users can use to always force a new bump. You could then implement additional checks in userland.

I think I would very much prefer this alternative option. Do you think this would allow you to solve your problem in userland?

I believe the option ** allowMultipleReleasesOfSameCommit** is a better choice since there is a complex logic to decide when to use --force. We can even make it per branch rule. I will update the PR soon.

… added

abdsahin · 2023-06-13T22:40:24Z

@mrgrain , I've updated the branch, let me know if it looks better, we merge or close it accordingly.

abdsahin and others added 2 commits April 19, 2023 09:37

fix(release): allow multiple tags on the same commit for different re…

841638b

…lease branches

Merge branch 'main' into fix/same-commit-multi-tag

da46e63

Merge branch 'main' into fix/same-commit-multi-tag

117cf6b

mrgrain requested changes Apr 20, 2023

View reviewed changes

fix(release): refoctor the code for same commit - multi tag logic

9f56d0c

abdsahin force-pushed the fix/same-commit-multi-tag branch from 0f34ab0 to 9f56d0c Compare April 20, 2023 21:16

Merge branch 'main' into fix/same-commit-multi-tag

7840dab

abdsahin added 3 commits April 23, 2023 22:12

Merge branch 'main' into fix/same-commit-multi-tag

c537527

Merge branch 'main' into fix/same-commit-multi-tag

5bf706e

Merge branch 'main' into fix/same-commit-multi-tag

64c923e

mrgrain reviewed Apr 25, 2023

View reviewed changes

mrgrain requested changes Apr 25, 2023

View reviewed changes

abdsahin and others added 4 commits May 11, 2023 17:38

Merge branch 'projen:main' into fix/same-commit-multi-tag

a332b0d

Merge branch 'main' into fix/same-commit-multi-tag

52a65f9

fix(release): separate logic for checking branches during release

f8d9250

fix(release): releaseSameCommitOnDifferentBranch optional property is…

6b2c8b8

… added

abdsahin requested a review from mrgrain June 14, 2023 08:39

abdsahin marked this pull request as draft June 14, 2023 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(release): allow multiple tags on the same commit for separate branches #2621

fix(release): allow multiple tags on the same commit for separate branches #2621

abdsahin commented Apr 19, 2023

mrgrain commented Apr 19, 2023

abdsahin commented Apr 19, 2023

mrgrain commented Apr 19, 2023

abdsahin commented Apr 19, 2023

abdsahin commented Apr 20, 2023

mrgrain left a comment

mrgrain Apr 20, 2023

abdsahin Apr 20, 2023

codecov-commenter commented Apr 20, 2023 •

edited

abdsahin commented Apr 23, 2023

mrgrain Apr 25, 2023

abdsahin Jun 13, 2023

mrgrain left a comment

mrgrain Apr 25, 2023

abdsahin Jun 13, 2023

mrgrain commented Apr 25, 2023

abdsahin commented Apr 26, 2023

abdsahin commented Jun 13, 2023

fix(release): allow multiple tags on the same commit for separate branches #2621

Are you sure you want to change the base?

fix(release): allow multiple tags on the same commit for separate branches #2621

Conversation

abdsahin commented Apr 19, 2023

mrgrain commented Apr 19, 2023

abdsahin commented Apr 19, 2023

mrgrain commented Apr 19, 2023

abdsahin commented Apr 19, 2023

abdsahin commented Apr 20, 2023

mrgrain left a comment

Choose a reason for hiding this comment

mrgrain Apr 20, 2023

Choose a reason for hiding this comment

abdsahin Apr 20, 2023

Choose a reason for hiding this comment

codecov-commenter commented Apr 20, 2023 • edited

Codecov Report

abdsahin commented Apr 23, 2023

mrgrain Apr 25, 2023

Choose a reason for hiding this comment

abdsahin Jun 13, 2023

Choose a reason for hiding this comment

mrgrain left a comment

Choose a reason for hiding this comment

mrgrain Apr 25, 2023

Choose a reason for hiding this comment

abdsahin Jun 13, 2023

Choose a reason for hiding this comment

mrgrain commented Apr 25, 2023

abdsahin commented Apr 26, 2023

abdsahin commented Jun 13, 2023

codecov-commenter commented Apr 20, 2023 •

edited