-
Notifications
You must be signed in to change notification settings - Fork 234
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
Yes sure, will update the PR when the review is completed :) |
@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 @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. |
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
Types of changes
Checklist: