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

build: Remove repeated steps for develop branch in workflow #3216

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

allgandalf
Copy link
Collaborator

@allgandalf allgandalf commented Sep 9, 2023

Description

This PR resolved #3187

I was proposing that we skip the Renderbook and CI tests on it's merge with the develop branch as the tests would already have been passed in the merge group.

No need to update the Docker workflow as it will publish the latest images on DockerHub once we merge the PR onto the develop branch.

But the workflow for CI and Renderbook might be skipped as for CI we do not publish it anywhere and for Renderbook, it gets published to github in the merge queue itself (Example).

Motivation and Context

Currently the workflow goes like, on approval of a PR, we go on to put the PR in the merge group where workflow will get triggered and then when the PR will get merged, a second workflow will get triggered on merger into the develop branch.

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@infotroph infotroph left a comment

Choose a reason for hiding this comment

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

Thanks for your patience on this, @RohanSasne -- I should have looked at this a long time ago.

I was proposing that we skip the Renderbook and CI tests on it's merge with the develop branch as the tests would already have been passed in the merge group.

The catch here is that the renderbook job does run on all PRs, but it only deploys from master and develop* -- note the if: github.event_name != 'pull_request' on lines 55 and 63 of book.yml. So as written this would make us only update the documentation when we push to master for official releases. We could certainly change the deployment rule to make the deployment happen in the merge group if we wanted, but we'd need another way to avoid deploying documentation changes from PRs that aren't merged.

*(Technically it only deploys from "anything that's not a PR", but we should change that -- see #3152)

I'm also cautious about skipping the CI job after merge -- yes, it's theoretically a duplicate of the tests in the merge group, but it does on occasion (once a year or so, maybe?) catch failures that didn't show up in the PR for one reason or another. I'll defer to @robkooper on whether those catches have been worth the daily extra CI runtime.

@allgandalf
Copy link
Collaborator Author

I'm also cautious about skipping the CI job after merge -- yes, it's theoretically a duplicate of the tests in the merge group, but it does on occasion (once a year or so, maybe?) catch failures that didn't show up in the PR for one reason or another. I'll defer to @robkooper on whether those catches have been worth the daily extra CI runtime.

Yes sure, will update the PR when the review is completed :)

@mdietze
Copy link
Member

mdietze commented May 16, 2024

@GandalfGwaihir @infotroph wanted to check in on the status of this PR and the requested changes. Is this something that can be wrapped up soon?

@infotroph
Copy link
Member

@mdietze @GandalfGwaihir From my perspective this is waiting for changes that address my concerns above, particularly about book.yml -- I'm confident that this were if merged as-is we'd break book deployment.

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.

Running CI and Renderbook workflow only for merge request, merge queue and Master Branch
3 participants