Skip to content
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

Merge specialized PR workflows back into one configuration file #2363

Merged
merged 27 commits into from
Jun 5, 2024

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented May 23, 2024

This refactors our PR builds so that we move back into a single workflow.

This PR removes the specialized -docs.yml workflows while retaining the fast path for only doc changes.

A follow up PR will also remove the companion -reporter.yml workflows.

This PR also rearranges our job dependencies a tad to be a tad more ecologically friendly 🌴 by ensuring we don't fail on formatting changes first before kicking off the heavier integration tests. We can extend this later with more build and linting fast failures.

image

@Mpdreamz Mpdreamz requested a review from v1v May 24, 2024 09:04
@Mpdreamz Mpdreamz changed the title Attempt to simply docs only CI runs Merge specialized PR workflows back into one configuration file May 24, 2024
Copy link
Member

@v1v v1v left a comment

Choose a reason for hiding this comment

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

I'm biased toward the former approach with the paths-ignore—it's a native GitHub Actions approach. Unless I miss the rationale behind this change, I prefer to avoid using tj-actions/changed-files@v44 to reduce the requirements of a third-party dependency.

Besides, I see the number of conditionals at the step level has increased, which I think makes things too ad hoc and hard to debug.

I'm not blocking this, but could you help me understand whether the other changes can stay so we can keep the —docs pattern?

@Mpdreamz
Copy link
Member Author

Mpdreamz commented May 24, 2024

@v1v hear you on the if conditions and the path ignore being more native. It looks worse than it is most are only at the single job level, only for required jobs they need to be on each step to short circuit properly. Hoping to refactor these to a composite action requiring only a single if: in a follow up PR.

Through this I also hope to minimize the heavier tests we run on PR's e.g only run azure/iis/profiler tests on PR's if they have relevant changes while still always running them on main. That gets more complex quickly with the -<changeset>.yml pattern.

@Mpdreamz Mpdreamz requested a review from v1v May 27, 2024 09:19
@Mpdreamz
Copy link
Member Author

#2365 relates to this to further simplify our CI workflows configuration.

It removes the test composite action and a third party dependency test reporting workflow.

@Mpdreamz Mpdreamz mentioned this pull request May 28, 2024
stevejgordon
stevejgordon previously approved these changes Jun 4, 2024
Copy link
Contributor

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

LGTM

v1v
v1v previously approved these changes Jun 4, 2024
Copy link
Member

@v1v v1v left a comment

Choose a reason for hiding this comment

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

I'm not blocking this. However, I'm biased in using the native GitHub way as it was.

What's the genuine reason for changing this? IIUC, was the only-docs approach already working fine, or did I miss something? If it's about centralising one GH workflow with all the steps. IMO, specialised PR workflow is a better approach to reduce side effects if changing one and breaking all, and it is easy to maintain and read.

Also, I still think the step conditionals are a bit complicated, and I prefer job conditionals

We enabled back in the days a centralised post-job to act as the GitHub check branch protection:

That could help centralise all the conditionals in one place.

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Jun 4, 2024

Thanks @v1v the main reason was that I also wanted to make the azure tests conditional on change sets while still only being run after lint/build.

It could be a separate workflow being triggered by another: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-a-workflow-based-on-the-conclusion-of-another-workflow

@Mpdreamz Mpdreamz dismissed stale reviews from v1v and stevejgordon via 4ea74f5 June 4, 2024 08:46
v1v
v1v previously approved these changes Jun 4, 2024
Copy link
Member

@v1v v1v left a comment

Choose a reason for hiding this comment

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

LGTM, with just a minor comment

Copy link
Contributor

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

LGTM

@Mpdreamz Mpdreamz merged commit 5491bbb into main Jun 5, 2024
15 checks passed
@Mpdreamz Mpdreamz deleted the fix/simplify-docs-noop-builds branch June 5, 2024 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants