pecan icon indicating copy to clipboard operation
pecan copied to clipboard

build: Remove repeated steps for develop branch in workflow

Open allgandalf opened this issue 1 year ago • 3 comments

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
  • [x] When possible

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [x] 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.
  • [x] I have read the CONTRIBUTING document.
  • [ ] I have added tests to cover my changes.
  • [ ] All new and existing tests passed.

allgandalf avatar Sep 09 '23 16:09 allgandalf

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 :)

allgandalf avatar Nov 28 '23 18:11 allgandalf

@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?

mdietze avatar May 16 '24 18:05 mdietze

@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.

infotroph avatar May 17 '24 05:05 infotroph