-
Notifications
You must be signed in to change notification settings - Fork 59
ci: Add workflow for tagging stable commit #593
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
base: main
Are you sure you want to change the base?
Conversation
sabaini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @skoech thanks for this!
Some minor nitpickery inline
| # Allows you to run this workflow manually from the Actions tab | ||
| workflow_dispatch: null | ||
| # schedule: | ||
| # - cron: '0 0 * * MON,THU' # Runs biweekly on Tuesdays and Thursdays |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we will want to enable the cron line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not tag every push to release branch with the snap artefact they be building ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we will want to enable the cron line
Yes, I only disabled it for testing purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not tag every push to release branch with the snap artefact they be building ?
I don't quite understand what you mean by this, @UtkarshBhatthere . Could you elaborate some more? :)
The main aim of this job is only to tag the stable commit, how would tagging every push to the release branch work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can keep the "tag all pushes" thing to another PR. For documentation purposes, tagging the stable release commit alone is sufficient.
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read # Needed for commit search via the API | ||
| # Steps represent a sequence of tasks that will be executed as part of the job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aestethic/readability nitpickery: in general it would be good to line up comments similarly to the workflow statements (here as well as below)
| # ----------------------------------------------------------- | ||
| # 2. Find the first name under “channels:” | ||
| # 3. and then parse <release version> and <commit ID> | ||
| # ----------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment accurate? Its not looking for the first but the first line that contains stable right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, first line that includes /stable:, thanks!
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const { execSync } = require('child_process'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent needs one more whitespace :-)
| core.setFailed('Failed to parse channel line'); | ||
| return; | ||
| } | ||
| const [, flavour, version, commit] = m; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming nit: not sure about flavour, maybe channel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"flavour" here is grabbing the unique "name" assigned to the release; which is not technically the full "channel" name. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AnneCYH, you're right. It's taking the code name, i.e. Squid.
| core.info(`Channel line: "${channelRaw}"`); | ||
| // Parse version, and commit ID | ||
| const m = channelRaw.match( | ||
| /^\s*([^/]+)\/stable:\s+([^+\s]+)\+snap([a-f0-9]+)\s/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest to tighten RE a bit:
- channel name
[a-z]+ - version:
[0-9.]+
| - name: Verify commit exists | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent one less whitespace
| ); | ||
| const hit = commits.find(c => c.sha.startsWith(target)); | ||
| if (hit) { | ||
| core.info(`✅ Found commit: ${hit.sha} — ${hit.html_url}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I try to keep source code ASCII just for simplicity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, I'll get rid of the emojis. :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😢
802fe6c to
02fee74
Compare
Signed-off-by: Sharon Koech <sharon.koech@canonical.com>
Signed-off-by: Sharon Koech <sharon.koech@canonical.com>
Signed-off-by: Sharon Koech <sharon.koech@canonical.com>
Signed-off-by: Sharon Koech <sharon.koech@canonical.com>
b41f2b2 to
0caeeac
Compare
sabaini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey thanks again -- some commentary / requests inline 🙏
| # Allow manual trigger from Actions UI | ||
| workflow_dispatch: | ||
| schedule: | ||
| - cron: '0 0 * * MON,THU' # Runs on Tuesdays and Thursdays at midnight UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment not accurate
| # Install the MicroCeph snap | ||
| - name: Install MicroCeph snap | ||
| run: | | ||
| sudo snap install microceph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For debugging purposes I'd suggest to run snap info microceph right here
| core.setFailed('Could not find "channels:" in snap info output'); | ||
| return; | ||
| } | ||
| // Get the first non-empty channel line after the header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment not accurate
| } | ||
| // Get the first non-empty channel line after the header | ||
| const channelRaw = lines.slice(headerIdx + 1) | ||
| .find(l => l.includes('/stable:')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This finds the first line that has "/stable" in it. Can we assume that this is the correct stable release though, can we rely on the ordering?
When I run snap info microceph I get
...
channels:
squid/stable: 19.2.0+snapab139d4a1f 2025-06-26 (1393) 117MB -
squid/candidate: 19.2.0+snap4910cfbd93 2025-07-15 (1407) 117MB -
squid/beta: ↑
squid/edge: 19.2.1+snap54a98b3cec 2025-08-07 (1464) 118MB -
latest/stable: 18.2.4+snapc9f2b08f92 2024-09-19 (1139) 102MB -
latest/candidate: ↑
latest/beta: ↑
latest/edge: 19.2.1+snap54a98b3cec 2025-08-07 (1464) 118MB -
reef/stable: 18.2.4+snapc9f2b08f92 2024-09-02 (1139) 102MB -
reef/candidate: 18.2.4+snapa97ae91192 2024-11-12 (1234) 102MB -
reef/beta: ↑
reef/edge: ↑
quincy/stable: 0+git.4a608fc 2024-01-10 (793) 96MB -
...
So here the first line with squid/stable is indeed correct. Can we rely on that ordering though?
The other issue I see here is that we're only handling the /stable releases in the main branch (ie. the latest major release). It would be great if we could also auto-tag other stable releases -- at the moment this would be squid, reef, quincy. Those would correspond to the branches main, reef and quincy respectively.
Maybe the workflow could loop through all the foo/stable channels, exclude the one named "latest/stable" and look to tag in the main and named branches.
| core.setFailed('Failed to parse channel line'); | ||
| return; | ||
| } | ||
| const [, codeName, version, commit] = m; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is codename here referring to the release channel name ? (like squid, or quincy etc ?).
| const commitSha = '${{ steps.verify.outputs.full_sha }}'; | ||
| const codeName = '${{ steps.snap.outputs.codeName }}'; | ||
| const version = '${{ steps.snap.outputs.version }}'; | ||
| const stableTag = `v${version}+${codeName}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So from what I've read above, if you use v${Version}+${codeName} as the tag, an example tag could look like:
-> v19.2.0+squid right ?
This has a slight problem that the tag will change everytime the underlying ceph point release changes. for example: in future the tag could become:
-> v19.2.2+squid.
Now since the tag is changing based on ceph-release, one of us would have to manually go and pull a new version on the RTD dashboard based on the new tag.
I propose we do not use the full semantic version but just the constant bit (something like v19+squid) as it will stay constant throughout the squid release life. And every time this workflow re-tags a commit, the documentation will update automatically.
UtkarshBhatthere
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some thoughts and nitpicking.
Description
When a MicroCeph snap revision is promoted to stable, the corresponding Git commit is tagged manually.This PR adds a GitHub Actions workflow to help us automate updating Git tags (which we also use for marking our doc versions - and hopefully for release notes using the GitHub release feature). The workflow uses the
github-scriptaction, and will:The workflow will run on a regular cadence (twice a week).
Type of change
Delete options that are not relevant.
How has this been tested?
This workflow hasn't been tested. I am hoping to test it by triggering it manually first, before enabling the
scheduleevent and cron expressions.Contributor checklist
Please check that you have: