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

Add workflow for publishing container image #327

Merged
merged 2 commits into from
Apr 18, 2023

Conversation

YaSuenag
Copy link
Member

Pull Request

Relates to #252 and #326

Summary

Publish WebAPI container to GitHub Packages (container registry) when new release is published on GitHub release page. It would happen on both pre-release and official relaese.

  • This workflow publishes WebAPI container for linux/amd64 and for linux/arm64
  • Container image generated by workflow would have tags in below
    • pre-release: pre is added as suffix of container tag like <release tag>pre
    • official release: both release tag and latest

Note that this workflow would not be fired when official release is migrated from pre-release - it means container image of official release wouldn't be generated even if you uncheck the pre-release checkbox in existing release in edit mode.

Changes

  • Add workflow for publishing WebAPI container image

Checklist

  • Local Tests Passing?
  • CICD and Pipeline Tests Passing?
  • Added any new Tests?
  • Documentation Updates Made?
  • Are there any API Changes? If yes, please describe below.
  • This is not a breaking change. If it is, please describe it below.
    • Just add new workflow which is fired by GitHub release webhook.

Are there API Changes?

No

Is this a breaking change?

No

Anything else?

I want to discuss about following topics before merging:

  • As I wrote before, this workflow would not be fired when official release is migrated from pre-release. I need to add released trigger if it is needed.
  • I added pre as a suffix of container image tag if this workflow is fired by pre-release. Is this suffix is needed?
  • Container image in GitHub Packages would not be associated with release page. It is better to do that. For example, Maven package would be associated with the release, so we can see it in Assets: https://github.com/YaSuenag/carbon-aware-sdk/releases/tag/v99.0.8 But I haven't found any solutions / documents for that. I guess we need to get help from GHA / GHP experts.
  • We need to clarify release process which uses GitHub release / packages.

@YaSuenag YaSuenag requested a review from vaughanknight as a code owner March 25, 2023 11:48
@codecov-commenter
Copy link

Codecov Report

Merging #327 (a2f57ac) into dev (2db0f30) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #327   +/-   ##
=======================================
  Coverage   81.73%   81.73%           
=======================================
  Files          69       69           
  Lines        2392     2392           
  Branches      244      244           
=======================================
  Hits         1955     1955           
  Misses        355      355           
  Partials       82       82           

@danuw
Copy link
Collaborator

danuw commented Mar 27, 2023

That is great @YaSuenag . Is it possible to trigger from merging to Dev for pre-release please?
(Release would use same principal for main merges i.e. official releases)
We can discuss in the morning but I believe you could use GITHUB_REF or github.ref_name for name of the incoming branch if that helps

Is if ref is Dev, use pre release naming. Only triggers on merges to Dev or main
(We could trigger manually and in that case we branch name as package name or prefix but expire image after 7 days?)

Speak in the morning...

@YaSuenag
Copy link
Member Author

Is it possible to trigger from merging to Dev for pre-release please?
(Release would use same principal for main merges i.e. official releases)

Do you mean that you want to use push trigger rather than release?

  • push: It would be fired when commits to push. We can specify the branch we are interested.
  • release: It would be fired when new release is published on GitHub release. I expect its operation would happen manually on GitHub Web UI.

I think binary release should be happen from GitHub release webhook (= release trigger action). OTOH we can ship nightly build release from GitHub Packages built by GitHub Actions. It is out of scope of ADR-0011, but I agree to ship it for upstream developers and CASDK enthusiasts. I'd like to work for nightly build if it is needed.

We can discuss in the morning but I believe you could use GITHUB_REF or github.ref_name for name of the incoming branch if that helps

In context of release event, GITHUB_REF and github.ref_name points to release tag.

(We could trigger manually and in that case we branch name as package name or prefix but expire image after 7 days?)

Do you mean "expired" container images should be gone from GitHub Packages? We need to remove them manually and/or using GitHub Actions which are fired like a cron.

@danuw
Copy link
Collaborator

danuw commented Mar 28, 2023

Thank you for your response and chat this morning.
So to summarise for this PR:

  • rename to 2-pre-release.yml
  • focus on pre-release build: every PR merge into Dev (push into dev) :
    • generate a new container tagged with version (start with commit ID for now?)
    • push to GitHub Packages

Do you mean "expired" container images should be gone from GitHub Packages? We need to remove them manually and/or using GitHub Actions which are fired like a cron.

You are right, maybe we don't use expiry, maybe we limit to say the latest 2-5 packages (so old ones get automatically removes however many changes go into dev for a given period)

@YaSuenag, Is that clearer?

EDIT: 2-release.yml to 2-pre-release.yml

@YaSuenag
Copy link
Member Author

I updated my PR.

This would be fired when some commits are pushed to dev branch, then it publishes container image for both AMD64 and Arm64 to GitHub Packages. The image have two tags: sha-<short commit hash> and pre. You can see the artifacts on my packages page: https://github.com/YaSuenag/carbon-aware-sdk/pkgs/container/carbon-aware-sdk

And also, this workflow attempt to keep recent 3 images which have a tag starts with sha-. Older images would be removed automatically.

I believe this workflow works fine, but I want to test it on "real" repo because some configurations might be different from my repo - permissions of GHA and/or GHP, API endpoint of GHP, and so on... Note that the test would publish container image to package are in GSF CASDK.

@YaSuenag
Copy link
Member Author

YaSuenag commented Apr 3, 2023

bash script to retrieve package ID which should be removed is a bit complex. I think it is more simple if we can use PowerShell. You can see the example on my branch:
https://github.com/YaSuenag/carbon-aware-sdk/blob/cb8a43073c8d2b600b7fd0bd3b96a259049093fc/.github/workflows/2-pre-release.yaml#L52-L90

IMHO bash has more users than PowerShell, so it can get more help for this workflow if we write in bash. However PowerShell is not so minor, and also if most of the SDK maintainers are familiar of PowerShell, we can go through with PowerShell.

What do you think about using PowerShell in this proposal? I will use it in this PR if I do not hear any objections.

@danuw
Copy link
Collaborator

danuw commented Apr 3, 2023

Thank you @YaSuenag sorry I missed your message last week. (I thought I had looked out for it before the week end and yet missed it)
Will look shortly but thank you so much for the updates.

@danuw
Copy link
Collaborator

danuw commented Apr 14, 2023

Hi @YaSuenag sorry still on holiday and struggled to find time in front of a computer to test this. Especially to be able to comment on naming as well.

I am not sure I understood what the (powershell) script was for (in the latest comment). Can you explain further please?

Generally I think it is what we are after. So if I have no chance to comment further on naming and anything else we should look to merge and iterate as needed while we progress.

Thank you again very much for this and hope we get each of our general release steps in as we progress through our weekly.

@YaSuenag
Copy link
Member Author

I am not sure I understood what the (powershell) script was for (in the latest comment). Can you explain further please?

In release workflow, we have to pick up image IDs to keep. We need to keep recent N container images, it is linked to the commits. Multi-arch containers has 3 images (manifest, AMD64, Arm64 in this case), so we need to scan all of manifests to decide what container images should be kept.
For example:

  1. manifest for commit A (ID: 130)
    • container images for AMD64 (ID: 131)
    • container images for Arm64 (ID: 132)
  2. manifest for commit B (ID: 120)
    • container images for AMD64 (ID: 121)
    • container images for Arm64 (ID: 122)
  3. manifest for commit C (ID: 110)
    • container images for AMD64 (ID: 111)
    • container images for Arm64 (ID: 112)
  4. manifest for commit D (ID: 100)
    • container images for AMD64 (ID: 101)
    • container images for Arm64 (ID: 102)

If we need to keep recent 3 commits (1 - 3 in this example), we should remove no.4 images - it means we have to remove ID 100, 101, 102. Unfortunately container images for specific architecture (ID 101 and 102 in this case) are untagged. Thus we need to track all of images from the manifest (ID 100 in this case).

This is collection operation. we can do it on bash as you know, but I think it is complex. So I want to implement it in PowerShell. However I'm concerned whether there are any disadvantages in point of maintenance. (mainly in point of maintainer skill)

if I have no chance to comment further on naming and anything else we should look to merge and iterate as needed while we progress.

Agree. This PR is first step to implement ADR-0011, and it does not affect the SDK behavior and other workflows. We just need to decide to use bash or PowerShell before merging IMHO.

@danuw
Copy link
Collaborator

danuw commented Apr 18, 2023

LGTM. Approved

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