Skip to content

run our docker ci on gha runners directly #17442

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

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

Conversation

ewdurbin
Copy link
Member

No description provided.

@ewdurbin ewdurbin requested a review from a team as a code owner January 17, 2025 18:55
@ewdurbin ewdurbin force-pushed the testing_gha_runners branch 2 times, most recently from f0bc2ba to 5a0d26c Compare January 17, 2025 19:47
@ewdurbin ewdurbin force-pushed the testing_gha_runners branch from 1311420 to 095c511 Compare January 17, 2025 20:05
@ewdurbin ewdurbin force-pushed the testing_gha_runners branch from 095c511 to b7dd6e8 Compare January 17, 2025 20:10
@ewdurbin
Copy link
Member Author

Overall, this has proven pretty flaky... seemingly unrelated to anything we're doing in particular :/

Screenshot 2025-01-17 at 4 02 01 PM

Screenshot 2025-01-17 at 2 24 45 PM (1)

Screenshot 2025-01-17 at 2 34 17 PM

@ewdurbin
Copy link
Member Author

Screenshot 2025-01-18 at 10 26 07 AM

@miketheman
Copy link
Member

zizmor errors can probably be resolved easily with https://gha-update.readthedocs.io/

@miketheman
Copy link
Member

#18103 should unblock this

Signed-off-by: Mike Fiedler <[email protected]>
@miketheman miketheman added testing Test infrastructure and individual tests github_actions Pull requests that update GitHub Actions code labels May 9, 2025
di
di previously approved these changes May 9, 2025
save: true
context: .
cache-from: type=gha
cache-to: type=gha,mode=max
Copy link
Member

Choose a reason for hiding this comment

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

I think we need something like:

Suggested change
cache-to: type=gha,mode=max
cache-to: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository && '' || 'type=gha,mode=max' }}

Otherwise action runs on PRs from untrusted forks can overwrite this cache as well.

(cc @woodruffw, should zizmor flag this?)

Copy link
Member

Choose a reason for hiding this comment

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

Huh yeah, it should -- I'm actually confused as to why it didn't, since I have a coordinate for the push: true part of this:

https://github.com/woodruffw/zizmor/blob/9790cec2727f04fee3cbaef1725f910c114e343e/src/audit/cache_poisoning.rs#L183-L186

Copy link
Member

Choose a reason for hiding this comment

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

Oh never mind, I see why it didn't: it considers docker/build-push-action a "publish sink" but not a "cache source" at the moment, since I'm only detecting push: true and not the cache-to: ... field.

This is a shortcoming in the current audit, I'll file an issue 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks! Just to confirm, this suggestion resolves the issue, correct?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, although I'm murky on whether doing the same for cache-from is also needed: that's a sink rather than a source so I think it's okay, but I might be missing something.

Copy link
Member

Choose a reason for hiding this comment

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

I think we specifically want cache-from to always be permitted here, so PRs can take advantage of our central cache to speed up builds (but can't write to it).

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense!

var-lib-apt
root-cache-pip
root-npm
key: cache-${{ hashFiles('Dockerfile') }}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
key: cache-${{ hashFiles('Dockerfile') }}
key: cache-${{ hashFiles('Dockerfile') }}
lookup-only: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository }}

https://github.com/actions/cache/blob/5a3ec84eff668545956fd18022155c47e93e2684/README.md?plain=1#L78

@di di self-requested a review May 9, 2025 16:15
@di di dismissed their stale review May 9, 2025 16:16

stale

Signed-off-by: Mike Fiedler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code testing Test infrastructure and individual tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants