build: Remove repeated steps for develop branch in workflow

[allgandalf]

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.

[infotroph]

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]

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]

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

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