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

Update CI to use Github Environments #326

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

Conversation

muddyfish
Copy link
Contributor

Issue #, if available: N/A

Description of changes:

Use github environments to separate the different security boundaries we have: an 'untrusted' account for third party forks, and a 'trusted' account for direct pushes and merges. The 'trusted' account is then used as a source for releases, and it will only run code that's already merged into the main repository.

Testing E2E: https://github.com/muddyfish/mountpoint-s3-csi-driver/actions/runs/12412282111
Testing release: https://github.com/muddyfish/mountpoint-s3-csi-driver/actions/runs/12411681334

Before Merging: Need to create GitHub environments on main repository.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@unexge unexge left a comment

Choose a reason for hiding this comment

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

Left some comments, overall looks good to me. Thanks for this!

uses: ./.github/workflows/e2e-tests.yaml
with:
environment: "untrusted"
ref: ${{ github.sha }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like were using ${{ github.event_name == 'push' && github.sha || github.event.pull_request.head.sha }} previously to get sha from pull requests. Not sure what was the reason for that. Is github.sha fine now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've not actually tried this from PRs - I'll try testing it

permissions:
id-token: write
contents: read
env:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this! It's much better than exporting variables in each step.

.github/workflows/e2e-tests.yaml Show resolved Hide resolved
.github/workflows/e2e-tests.yaml Show resolved Hide resolved
.github/workflows/e2e-test-untrusted.yaml Show resolved Hide resolved
on:
push:
branches: [ "main", "feature/**", "release-**", "workflow/**" ]
merge_group:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if that was possible before but seems like we can also use merge queues now. That's great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not completely certain if this works, it was actually in the previous thing as well. I think merge queues are a thing we need to enable in the repository settings

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but this definition in the workflow ensures that the checks will run and then when we switch it on in repo settings, it will then allow the merge queue to run and read those workflows.

.github/workflows/release.yaml Outdated Show resolved Hide resolved
.github/workflows/release.yaml Show resolved Hide resolved
.github/workflows/release.yaml Outdated Show resolved Hide resolved
@muddyfish muddyfish force-pushed the feature/ci-update branch 2 times, most recently from f6098d4 to 15c5bfc Compare December 20, 2024 10:23
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