Skip to content

CI: Wait for static check results before starting builds - #65232

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
yedpodtrzitko:yed/ci-entry-workflow
Apr 5, 2023
Merged

CI: Wait for static check results before starting builds#65232
akien-mga merged 1 commit into
godotengine:masterfrom
yedpodtrzitko:yed/ci-entry-workflow

Conversation

@yedpodtrzitko

@yedpodtrzitko yedpodtrzitko commented Sep 2, 2022

Copy link
Copy Markdown
Contributor

// Another attempt for what did not work in #65123

The CI workflow "📊 Static Checks" checks code formatting etc. and even if all the build workflows in pipeline are successful, the pipeline needs to be eventually re-run in case this one fails. From what I understand the bottleneck for the CI pipeline is the number of available workers which can run builds, so it would make sense to not run the "expensive" jobs unless the basic one is successful.

Unfortunately it's impossible to make various workflow files depend on each other, so the approach here utilizes Reusable Workflows:

  • change the trigger event for all builders to workflow_call, which means they need to be invoked manually
  • create new workflow file runner.yml as an entrypoint for the pipeline
  • in there, invoke "📊 Static Checks" first
  • run other builds if 📊 Static Checks job was successful

This means builders wont be shown separately in the Actions Tab, since they are now treated as a part of the Runner workflow. That makes it easier to cancel the whole build with a single click (a minuscule case, but still a nice thing to have) and it declutters the content there to a single row only per each git push. But that also means you cant see or filter the build types (ios, android...) there directly.

@umarcor umarcor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FTR, I independently proposed almost the same modification (#75338). The only difference is a few names.

@YuriSizov

Copy link
Copy Markdown
Contributor

@yedpodtrzitko Can you rebase this for a good measure?

@yedpodtrzitko
yedpodtrzitko force-pushed the yed/ci-entry-workflow branch from f8fbd41 to 36beb34 Compare April 4, 2023 13:57
Comment thread .github/workflows/runner.yml Outdated
Comment thread .github/workflows/runner.yml Outdated
Comment thread .github/workflows/runner.yml Outdated
Comment thread .github/workflows/runner.yml Outdated
Comment thread .github/workflows/runner.yml Outdated
Comment thread .github/workflows/runner.yml Outdated
Comment thread .github/workflows/runner.yml Outdated
Comment thread .github/workflows/runner.yml Outdated
@yedpodtrzitko

Copy link
Copy Markdown
Contributor Author

@YuriSizov done

@umarcor umarcor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As discussed in #75338, please make workflow/job names shorter to reduce overflow in web views.

@yedpodtrzitko

Copy link
Copy Markdown
Contributor Author

@umarcor regarding shorter names - this is discussed in #75333 and I couldnt find there any compeling reason to do the renames other than for the sake of doing the renames. Did I miss your answer there?

@umarcor

umarcor commented Apr 4, 2023

Copy link
Copy Markdown
Contributor

@yedpodtrzitko click "Show all checks" at the bottom of this PR. Or see the sidebar of https://github.com/godotengine/godot/actions/runs/4608607317.

Currently, this PR drops the icons and a third of the space is used by 'Tiered Pipeline Runner'.

@yedpodtrzitko

yedpodtrzitko commented Apr 4, 2023

Copy link
Copy Markdown
Contributor Author

I rest my case. I'm just gonna do the rebase as requested and leave the decision to maintainers.

@umarcor

umarcor commented Apr 4, 2023

Copy link
Copy Markdown
Contributor

@yedpodtrzitko fair enough. Please do edit the first message to explain the reason for dropping the icons.

@umarcor

umarcor commented Apr 4, 2023

Copy link
Copy Markdown
Contributor

For clarification, #75333 is about the priority of the hardcoded/static content in parens vs the dynamically generated content also in parens or in italics. This PR is introducing a prefix that hides both the static and the dynamic content.

@YuriSizov

YuriSizov commented Apr 4, 2023

Copy link
Copy Markdown
Contributor

I'm just gonna do the rebase as requested and leave the decision to maintainers.

For the record, Akien and I agree that we shouldn't lose the icons and that we should use short names so that the checklist is readable. So suggestions by @umarcor above are indeed welcome.

@yedpodtrzitko
yedpodtrzitko force-pushed the yed/ci-entry-workflow branch from 9b3779e to 93b7bcb Compare April 4, 2023 15:05
@yedpodtrzitko

yedpodtrzitko commented Apr 4, 2023

Copy link
Copy Markdown
Contributor Author

ok it should be done. thanks @umarcor

@akien-mga akien-mga modified the milestones: 4.x, 4.1 Apr 5, 2023
@akien-mga akien-mga added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release cherrypick:4.0 labels Apr 5, 2023
@akien-mga
akien-mga merged commit 1a77d37 into godotengine:master Apr 5, 2023
@akien-mga

Copy link
Copy Markdown
Member

Thanks!

@YuriSizov

Copy link
Copy Markdown
Contributor

Cherry-picked for 4.0.3.

@akien-mga akien-mga changed the title ci: wait for static check results before starting builds CI: Wait for static check results before starting builds May 12, 2023
@akien-mga

Copy link
Copy Markdown
Member

Cherry-picked for 3.6.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 29, 2023
@akien-mga

Copy link
Copy Markdown
Member

Cherry-picked for 3.5.3.

@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Sep 6, 2023
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.

5 participants