build: Remove repeated steps for develop branch in workflow

allgandalf commented 9 months ago

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 commented 7 months ago

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 commented 1 month ago

@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 commented 1 month ago

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

PecanProject / pecan