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.

Sign up for GitHub

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

Jump to bottom

Wrong checkout branch in the changelog action? #597

Open
germa89 opened this issue Oct 3, 2024 · 5 comments · May be fixed by #598
Open

Wrong checkout branch in the changelog action? #597

germa89 opened this issue Oct 3, 2024 · 5 comments · May be fixed by #598

Comments

@germa89
Copy link
Contributor

germa89 commented Oct 3, 2024

\begin{rant}

Sooo..... I have been having some issues with my release today (as always)....

I noticed, that:

actions/doc-deploy-changelog/action.yml

Lines 321 to 341 in d294898

run: |
# Checkout main & create a branch
git checkout ${{ env.MAIN_BRANCH }}
git pull
# Set branch for PR
pr_branch=maint/v${{ env.TAG_VERSION }}-changelog
# Check if pr_branch exists on remote
remote_branch_exists=$(git ls-remote --heads origin refs/heads/$pr_branch 2>&1)
# Delete the remote branch if it exists
if [ -n "$remote_branch_exists" ]; then
echo "Deleting $pr_branch from remote"
git push origin --delete $pr_branch
fi
# Create a new $pr_branch
git checkout -b $pr_branch
cherrypick=$(git cherry-pick ${{ env.COMMIT_SHA }} >> cherrypickerr.txt 2>&1 || true)

The branch pr_branch=maint/v${{ env.TAG_VERSION }}-changelog is created FROM main. See:

actions/doc-deploy-changelog/action.yml

Line 339 in d294898

git checkout -b $pr_branch

I do not think it should be like that... it should be from the tagged branch (release/v*** branch, or main if you are tagging main) to avoid conflicts in case you have to do quick commits to the release branch.

You might think "it is a bad practice commit to release branch"... I know but sometimes it is needed.

Context

  1. The begining.
    I had changelog rendering issues because some PR title were:

    docs: adding warning about *mwrite. Update *vwrite warning to include *mwrite

    Which of course, renders pretty badly in markdown... Don't blame me... it is MAPDL command style....

  2. The "quick fix".
    So I said... "OOkk.... The bot already opened the PR for the changelog in main, and I already merged it... okkkk well job German"....
    (NOTE_1: can we change that?? to not open the PR updating main until the release is done?)....
    I thought, "Ok... i will change quickly the markdown on the release branch, and I wil re-tag...." .. Oh man, I was naive.

  3. The disaster.
    Because what I mention above... the maint/v****-changelog branch being created from main... I have cherry-picking conflicts....

  4. The solution.
    I guess I could go to main, revert the changelog PR (to get back the changelog files), do the markdown changes there, and merge again in release... but that will bring the full commit history with it (changelog PR and its revert)... a mess....

\end{rant}

Requests

If possible, I would like to see the follwing changes:

  • Create maint/v${{ env.TAG_VERSION }}-changelog from the tagged branch (release or main)
  • [Extra] Delay opening changelog PR to main until the release is done.
@germa89
Copy link
Contributor Author

germa89 commented Oct 3, 2024

Pinging @klmcadams @RobPasMue

@germa89 germa89 linked a pull request Oct 3, 2024 that will close this issue
Open
@germa89
Copy link
Contributor Author

germa89 commented Oct 3, 2024

The [extra] cannot be done easily: actions/runner#1478

@klmcadams
Copy link
Contributor

The reason why maint/v${{ env.TAG_VERSION }}-changelog was created from main was to make sure the branch only contained the cherry-picked changelog changes.

If you made maint/v${{ env.TAG_VERSION }}-changelog from the release branch, then I don't think you would have to cherry pick the changes. You could just make a PR into main since the release branch should already have the updated changelog. Technically, the only difference between the release branch and main should be the missing towncrier files, right? There's not a chance that there could be other code changes in the release branch that aren't reflected in main? If so, I think it would be fine to make maint/v${{ env.TAG_VERSION }}-changelog from the tagged branch (release or main).

In the case of a tag being created from a release branch, I think these would be the steps instead of cherry picking:

  • Create maint/v${{ env.TAG_VERSION }}-changelog from the release branch
  • Run the gh pr create command, now making sure to specify the base branch (the target - main)

Also, I agree that waiting until the release is done before the PR is created would be tricky. We would need to add a step to the release actions to check for the PR branch in the repository, & if it exists, make a PR into main (or something like that)

@RobPasMue
Copy link
Member

Technically, the only difference between the release branch and main should be the missing towncrier files, right? There's not a chance that there could be other code changes in the release branch that aren't reflected in main?

There might - I wouldn't perform this assumption @klmcadams - sometimes hot fixes are needed on release branches that are not reflected on main.

The [extra] cannot be done easily

This is hard to control IMO - I would just "hold" merging it until the release is done so that you can close it in case something went wrong... You shouldn't be merging it until you are 100% sure your release went well - that's why we open a PR and not automatically merge to main @germa89 😄

@germa89
Copy link
Contributor Author

germa89 commented Oct 4, 2024 •
edited
Loading

I think your logic @klmcadams is right.. in theory, it should be OK.. but as @RobPasMue mentioned, sometimes you need to do small fix in release branches. Ofc this is not desirable, nor recommended, but sometimes you have to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: creating branch from tagged branch ansys/actions
3 participants
@germa89 @RobPasMue @klmcadams