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

Refine snap ci workflow and improve resiliency #452

Merged
merged 7 commits into from
Feb 23, 2024
Merged

Conversation

tim-hm
Copy link
Contributor

@tim-hm tim-hm commented Feb 21, 2024

No description provided.

@tim-hm tim-hm requested a review from d-loose as a code owner February 21, 2024 15:43
@tim-hm tim-hm requested a review from spydon February 21, 2024 15:43
.github/workflows/pr.yaml Outdated Show resolved Hide resolved
ci/snap-build-setup.sh Outdated Show resolved Hide resolved
git add -A
git commit --amend -m "$COMMIT_MSG" --author="GitHub Actions <[email protected]>"
git commit --amend --no-edit # keep original author but set committer to [email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're changing the SHA anyways, isn't it better to just create a new commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We touched on this during standup and both approaches have flaws. If you add a commit then the message show in launchpad isn't helpful (ie all you see is something like ci: prep for launchpad snap build). If you modify the HEAD then it's not signed but at least the commit message matches what is in the git history.

I'm happy either way, so let me know if you have a strong opinion either way!

Copy link
Member

Choose a reason for hiding this comment

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

In any case, we should introduce meaningful version numbers for the snaps then, as the associated commit hash will never be part of main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh so many issues all because we can't simply do snapcraft -f path-to-thing.yaml! Lets discuss at standup. I'd like to maintain the sha over version nos.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could just add the last commit message to the new commit. i.e.

ci(release): <Last commit message>

Copy link
Contributor Author

@tim-hm tim-hm Feb 23, 2024

Choose a reason for hiding this comment

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

Correct me if I'm off, but is the aim to be able to trace the build artefact back to a specific commit sha on main?

If so, then piling release commits on ci/name might work at the start, but over time the branch's history would become long and problematic. For example, after 50 releases you'd have to go quite far back in the launchpad history to find the HEAD of main -- although it would still be possible! If we amend to avoid stacking commits, the commit in ci/<name> would be overridden on the next invocation and so tracing back to a specific sha would only be a matter of looking at HEAD^1 in launchpads history.

I think the solution we actually want is to combine a few bits here:

  • I like your updated commit message so will adopt that
  • I think we can use external-metadata to get info from HEAD^1 to properly version the artefact.
  • go with amending but include details in the subject/commit to reflect the main commit it was built from

WDTY?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If so, then piling release commits on ci/name might work at the start

Yeah, that doesn't sound like a good solution.

I like your updated commit message so will adopt that

In a new commit right? Maybe we should add the SHA of the previous commit in the commit message too, then we'd be able to track it "properly", or maybe not worth it since it'll always be HEAD~1?

I think we can use external-metadata to get info from HEAD^1 to properly version the artefact.

Sounds good, we should probably start running melos version as part of this workflow too and get the version from there.

Copy link
Member

Choose a reason for hiding this comment

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

A dedicated release commit message would be great 👍
Would it be terribly misleading if we used snapcraftctl set-version <hash of HEAD~>?
I'm also not opposed to the melos version idea :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep agree with all of this. I've created #456 to get that work done if you're happy to punt that challenge for the moment?

Tim Holmes-Mitra added 7 commits February 23, 2024 12:47
Snapcraft.yamls and artefacts were in the /snap/* folder. This meant
that running `snapcraft` gave an error. It was also brittle when running
local builds due to individual files within snap/ needing to be move
around (rather than simply being able to remove the folder and copy a
fresh one from somewhere else).

This restructure avoids the warning and provides a straightforward way
to prepare for a snap build:

```bash
$ ./ci/snap-build-setup.sh bootstrap # or
$ ./ci/snap-build-setup.sh init
```
Given that this workflow positions snapcraft.yaml and artefacts for a
launchpad build it can be forceful in its merging strategy. Thus,
the rebase-for-snap-cicd workflow now uses:

1. A matrix strategy to merge all rebase-for-snaps jobs into a single job
2. Leverages `./ci/snap-build-setup.sh <name>` to structure the repo
3. Uses `git reset --hard main` rather than a rebase
4. Amends HEAD on `ci/(init|bootstrap)` and adds GitHub Actions as the
comitter. This means that when visually scanning commits in Launchpad
these better align to what is seen in GitHub.
Building locally is brittle due to artefacts polluting the build
container (caused by `source: .`). This doesn't present in ci because
the build container is fresh.

The flutter-git part, now checks if a previous clone exists to avoid
a failure if the snapcraft build is run twice. Additionally,
CONTRIBUTING.md highlights the difficulties with local builds and
provides some guidance on workarounds.
The `contact` field is displayed in three places: `snap info <name>`,
the App Center, and snapstore.io. When set, the snapstore.io publisher
page overrides the `snapcraft.yaml` contact value.

The `snapcraft.yaml`'s `issue` key does not appear anywhere, but if set
in the snapstore.io publisher page, then it is displayed at
`snapstore.io/<snap-name>` under "Report a bug".

Going forward, we should prefer setting these values in the publisher
page and omit them from the `snapcraft.yaml` to avoid confusion.

Signed-off-by: Tim Holmes-Mitra <[email protected]>
@tim-hm tim-hm merged commit 4f75986 into canonical:main Feb 23, 2024
6 of 15 checks passed
github-actions bot pushed a commit that referenced this pull request Feb 23, 2024
github-actions bot pushed a commit that referenced this pull request Feb 23, 2024
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