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

fix: allow local building and testing of the snap on PRs #326

Merged
merged 1 commit into from
Nov 19, 2023

Conversation

cjdcordeiro
Copy link
Collaborator

The snap workflows can't currently run for PRs created from forks, since the repo's policy is not to allow forks to access any secrets.

The caveat here is that, for PRs, we won't be able to assert if the snap can build for architectures other than the GH's runner.arch. I don't think there's a way around this since the multi-arch build relies on LP, and for that we always need credentials.

To be fair, the same would happen if we still had the Snap store connected to this repo, since the multi-arch builds would also only happen on push to master.

In this PR

The snap.yaml workflow builds the snap with basis on the type of event triggering the workflow:

  • for PRs, the snap build will only take place for amd64, and use snapcraft pack
  • for every other event (push and release), the snap build will rely on LP (i.e. snapcraft remote-build)

.github/workflows/snap.yml Outdated Show resolved Hide resolved
.github/workflows/snap.yml Outdated Show resolved Hide resolved
@cjdcordeiro cjdcordeiro force-pushed the test-snap-on-prs branch 8 times, most recently from 45ba848 to 9749b41 Compare November 13, 2023 14:28
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. @jnsgruk Want to take a look?

@benhoyt benhoyt merged commit c974cd5 into canonical:master Nov 19, 2023
15 checks passed
@benhoyt
Copy link
Contributor

benhoyt commented Nov 19, 2023

Looks good to me. Merged without further review -- this is CI only so easy to tweak later if necessary.

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.

2 participants