Skip to content

Conversation

@XxdpavelxX
Copy link
Contributor

@XxdpavelxX XxdpavelxX commented Sep 5, 2025

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@XxdpavelxX XxdpavelxX self-assigned this Sep 5, 2025
@XxdpavelxX XxdpavelxX changed the title INFRA-2911-Skip generating commits.dvs for hotfixes INFRA-2911-Skip generating commits.csv for hotfixes Sep 6, 2025
Copy link
Contributor

@gauthierpetetin gauthierpetetin left a comment

Choose a reason for hiding this comment

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

Thank you for this new PR @XxdpavelxX , I added a few comments

fi


# Note: Skip PREVIOUS_VERSION_REF validation to allow empty for hotfixes
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we place this comment here? It seems we don't have related logic close to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just a general comment to explain the behavior for hotfixes, but I'll remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok!

git fetch origin "${previous_version_ref}"
DIFF_BASE="origin/${previous_version_ref}"
# Skip commits.csv for hotfix releases (previous_version_ref empty)
local skip_csv=false
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need skip_csv variable for our 'if' conditions below, since we can also base our 'if' conditions on the state of previous_version_ref variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can replace conditionals using skip_csv=true/false with previous_version_ref='null' if that works?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that works for me, and we could add a detailed comment to explain why, such as:

- When we create a new major/minor releases, we fetch all commits included in the release, by fetching the diff between HEAD and previous version reference.
- When we create a new hotfix releases, there are no commits included in the release by default (they will be cherry-picked one by one). So we don't have previous version reference, which is why the value is set to 'null'.

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


# Step 3: Create version bump PR for main branch
create_version_bump_pr "$PLATFORM" "$NEW_VERSION" "$next_version" "$version_bump_branch_name" "$release_branch_name" "main"
create_version_bump_pr "$PLATFORM" "$NEW_VERSION" "$next_version" "$version_bump_branch_name" "$release_branch_name" "$BASE_BRANCH"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can pass "BASE_BRANCH" instead of "main" here, as "BASE_BRANCH" is meant to be the base branch of the release PR, which is always set to stable.

Copy link
Contributor Author

@XxdpavelxX XxdpavelxX Sep 8, 2025

Choose a reason for hiding this comment

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

Good catch. A little confused though, should I hardcode it to "main" or "stable" here?

Copy link
Contributor

Choose a reason for hiding this comment

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

You should hardcode it to "main", as we'll bump the version on main.

Signed-off-by: Pavel Dvorkin <pavel.dvorkin@consensys.net>
@XxdpavelxX XxdpavelxX force-pushed the INFRA-2911-AllowNullPreviousVersionRef branch from c3a0059 to bb2b66d Compare September 8, 2025 14:42
gauthierpetetin
gauthierpetetin previously approved these changes Sep 8, 2025
Copy link
Contributor

@gauthierpetetin gauthierpetetin left a comment

Choose a reason for hiding this comment

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

I added two nits to fix. Other than that LGTM

echo "Hotfix release detected (previous-version-ref empty); skipping commits.csv generation."
skip_csv=true
# Skip commits.csv for hotfix releases (previous_version_ref is literal "null")
# - When we create a new major/minor releases, we fetch all commits included in the release, by fetching the diff between HEAD and previous version reference.
Copy link
Contributor

Choose a reason for hiding this comment

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

a new major/minor releases --> release without an "s"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

skip_csv=true
# Skip commits.csv for hotfix releases (previous_version_ref is literal "null")
# - When we create a new major/minor releases, we fetch all commits included in the release, by fetching the diff between HEAD and previous version reference.
# - When we create a new hotfix releases, there are no commits included in the release by default (they will be cherry-picked one by one). So we don't have previous version reference, which is why the value is set to 'null'.
Copy link
Contributor

Choose a reason for hiding this comment

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

a new hotfix releases --> release without an "s"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@XxdpavelxX XxdpavelxX merged commit 432e82a into main Sep 8, 2025
19 checks passed
@XxdpavelxX XxdpavelxX deleted the INFRA-2911-AllowNullPreviousVersionRef branch September 8, 2025 19:13
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