GitHub action #21

ll-nick · 2024-11-07T16:20:49Z

This adds GitHub Actions to the repository that do the following:

Run the unit tests on all pushes to any branch
On a merge to main:
- Bump the version that's currently defined in the version file, add/commit the new version and git tag this commit. By default, this bumps the minor version but this can be adjusted using the #major, #minor, #patch keywords in the commit message.
- Pulls the newly created tag and builds the deb package and archive releases.
- Creates a new release on GitHub using the PR description as the release description.

An action using the GITHUB_TOKEN cannot trigger another workflow. So to create a new release after a version bump, we simply run the release in the same workflow. See https://github.com/orgs/community/discussions/27028#discussioncomment-3254360

orzechow

This is so awesome! 🤩

Two nitpicky points, but feel free to merge.

.github/workflows/bump-version-and-create-release.yaml

orzechow · 2024-11-08T11:22:03Z

.github/workflows/bump-version-and-create-release.yaml

+      - name: Create Release
+        uses: ncipollo/release-action@v1
+        with:
+          artifacts: "/tmp/artifacts/release/*"
+          tag: ${{ needs.bump-version.outputs.new_tag }}
+


It would be nice, if the release description would contain the corresponding PR description.
Currently, it's the version bump commit message 🙈

I imagine this could work:

save the merge commit message into a tmp file (must be an early github action step, before version bump commit)

pass this file to the ncipollo/release-action action as bodyFile

ll-nick · 2024-11-08T13:00:19Z

Thanks for the review and nice idea with the release body, let me look into that.

A couple of other points that came to my mind:

The readme should now suggest using the deb package as the recommended way of installing it.
It would be nice to have multi-architecture builds/releases. If that's not too complicated, I'd integrate that as well.
Right now, the default version bump is set to patch. Do you think that's appropriate or should we set it to minor @orzechow ?

orzechow

Sorry, I found a bug:

The latest release v0.1.1 contains the v0.1.0 package 😢
https://github.com/KIT-MRT/util_caching/releases/tag/v0.1.1

orzechow · 2024-11-08T14:49:09Z

The readme should now suggest using the deb package as the recommended way of installing it.

Good point!
Feel free to change it with this PR 👍

It would be nice to have multi-architecture builds/releases. If that's not too complicated, I'd integrate that as well.

Well, a header-only release is architecture independent – that's one of it's beauties.
But – I had a wrong CPack variable name which made CPack use the system architecture instead of setting architecture=all.
Fixed with #22

Right now, the default version bump is set to patch. Do you think that's appropriate or should we set it to minor @orzechow ?

Right. #minor would be more appropriate.

Co-authored-by: Piotr Spieker <[email protected]>

ll-nick · 2024-11-11T09:51:21Z

Ok @orzechow, this one is ready for another review.

I did merge a dummy change again to test the workflow. This successfully created a new release like we want it to.
Before merging, I'll delete that release and the dummy changes I made in cd459d5 to trigger the workflow.

The readme should now suggest using the deb package as the recommended way of installing it.

Good point!
Feel free to change it with this PR 👍

Done in b7d01be

It would be nice to have multi-architecture builds/releases. If that's not too complicated, I'd integrate that as well.

Well, a header-only release is architecture independent – that's one of it's beauties.
But – I had a wrong CPack variable name which made CPack use the system architecture instead of setting architecture=all.
Fixed with #22

Very good point, did not think that through 😅

Right now, the default version bump is set to patch. Do you think that's appropriate or should we set it to minor @orzechow ?

Right. #minor would be more appropriate.

Changed the default bump to #minor in c1486df

It would be nice, if the release description would contain the corresponding PR description.

Done in 93676b7. We should be aware that this is vulnerable to script injections but I figured that we would probably notice malicious code in the PR description.

Sorry, I found a bug:

The latest release v0.1.1 contains the v0.1.0 package 😢

Good catch! This happened because the docker build-push-action will use the repository state that caused the workflow, so changes inside the workflow are not considered. This means that the deb package did contain all the changes of the new version except the bumped version file which consequently lead to the wrong name of the package.

I fixed it by explicitly checking out the tag we created in the first job in 5adc18d and by setting the docker context to . in 45d3b44. See here for reference on why this is necessary.

ll-nick · 2024-11-11T11:29:19Z

Since the PR description will now be the release description too, I updated the original description of the PR to be a little more concise and highlight the contributions of the PR.

orzechow

Ok @orzechow, this one is ready for another review.

Beautiful, thanks a lot! 👏

Two suggestions, feel free to merge after considering these.

It would be nice, if the release description would contain the corresponding PR description.

Done in 93676b7. We should be aware that this is vulnerable to script injections but I figured that we would probably notice malicious code in the PR description.

Awesome, the test release v0.2.0 worked out great!

Conserning the vulnerability, I agree that we would probably notice such code.
But, I think we can avoid it altogether, see my code suggestions.

Sorry, I found a bug:
The latest release v0.1.1 contains the v0.1.0 package 😢

Good catch! This happened because the docker build-push-action will use the repository state that caused the workflow, so changes inside the workflow are not considered. This means that the deb package did contain all the changes of the new version except the bumped version file which consequently lead to the wrong name of the package.

I fixed it by explicitly checking out the tag we created in the first job in 5adc18d and by setting the docker context to . in 45d3b44. See here for reference on why this is necessary.

Nice and clean solution! 👍

.github/workflows/bump-version-and-create-release.yaml

orzechow · 2024-11-11T11:49:16Z

README.md

+Download the latest `.deb` package from the [releases page](https://github.com/KIT-MRT/util_caching/releases) and install it with `dpkg`:
+
+```bash
+sudo dpkg -i util-caching*.deb


Optional:
We could think of removing the version from the deb file name (by changing CPACK_DEBIAN_FILE_NAME).
That would enable a direct link here:

Suggested change

Download the latest `.deb` package from the [releases page](https://github.com/KIT-MRT/util_caching/releases) and install it with `dpkg`:

```bash

sudo dpkg -i util-caching*.deb

Download the [latest `.deb` package release](https://github.com/KIT-MRT/util_caching/releases/latest/download/util-caching_amd64.deb) and install it with `dpkg`:

```bash

sudo dpkg -i util-caching_amd64.deb

I've seen this in other projects, but not sure if that's good practise.
On the other hand, it also eases the integration in automated installs, like in docker.

Yes, let's do that. I think having a static link here is quite beneficial.

What about the architecture in the file name? As you mentioned, this is header only, so the deb file should work everywhere right?

Should I just set

set(CPACK_DEBIAN_FILE_NAME "${PROJECT_NAME}.deb")

in cpack_config.cmake?

What about the architecture in the file name? As you mentioned, this is header only, so the deb file should work everywhere right?

Right, let's then call the packages and deb files -dev. That should follow the debian style as well (except for the dropped version).

I'll provide a change for that.

libutil-caching-dev?

See 490ded5 and 124f963

Yes, that sounds about right. 👍

Simplify the way the release body is defined. Co-authored-by: Piotr Spieker <[email protected]>

ll-nick added 8 commits November 7, 2024 13:11

Use WORKDIR to simplify Dockerfile

597b243

Add Docker stage to build release artifacts

6db1afa

Add Docker stage to run unit tests

ea6d843

Add workflow to run unit tests

f097f81

Create action to create a release

1cd81c6

Add workflow to auto-bump the version

37f6cbf

Copy release artifacts differently due to deadlock when using outputs

2baee09

See moby/buildkit#2950

ll-nick requested a review from orzechow November 7, 2024 16:20

ll-nick self-assigned this Nov 7, 2024

orzechow approved these changes Nov 8, 2024

View reviewed changes

orzechow self-requested a review November 8, 2024 14:35

orzechow requested changes Nov 8, 2024

View reviewed changes

ll-nick force-pushed the github_action branch from 2104b3c to 148b9d1 Compare November 11, 2024 08:44

ll-nick and others added 5 commits November 11, 2024 09:44

Set VERSION env in a more reuseable manner

636940c

Co-authored-by: Piotr Spieker <[email protected]>

Add note about deb package in README install section

b7d01be

Set default version bump to minor

c1486df

Explicitly checkout new tag before running release build

5adc18d

Add the PR description as release body

93676b7

ll-nick force-pushed the github_action branch from 497f272 to 93676b7 Compare November 11, 2024 09:15

Set context to fix git context limitation of docker build action

45d3b44

ll-nick force-pushed the github_action branch from 9e7d84b to 45d3b44 Compare November 11, 2024 09:32

ll-nick requested a review from orzechow November 11, 2024 09:55

orzechow linked an issue Nov 11, 2024 that may be closed by this pull request

Deploy as debian package #8

Closed

2 tasks

orzechow approved these changes Nov 11, 2024

View reviewed changes

Update CMake project description following the Readme change in 8d8b643

6e22a2e

ll-nick force-pushed the github_action branch from 1997e0d to 6e22a2e Compare November 11, 2024 12:09

Apply suggestions from code review

c0e3685

Simplify the way the release body is defined. Co-authored-by: Piotr Spieker <[email protected]>

ll-nick force-pushed the github_action branch from e0655ce to c0e3685 Compare November 11, 2024 12:40

orzechow and others added 3 commits November 11, 2024 13:50

Rename debian package to libutil-caching-dev

71e4eaf

Use fixed name for debian package file (without version)

3407cdd

Remove dev branch from workflow triggers

833222e

ll-nick force-pushed the github_action branch from 124f963 to 833222e Compare November 11, 2024 14:13

ll-nick merged commit 2688bd5 into main Nov 11, 2024
1 check passed

ll-nick deleted the github_action branch November 11, 2024 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub action #21

GitHub action #21

ll-nick commented Nov 7, 2024 •

edited by orzechow

Loading

orzechow left a comment

orzechow Nov 8, 2024

ll-nick commented Nov 8, 2024

orzechow left a comment

orzechow commented Nov 8, 2024

ll-nick commented Nov 11, 2024 •

edited

Loading

ll-nick commented Nov 11, 2024

orzechow left a comment

orzechow Nov 11, 2024 •

edited

Loading

ll-nick Nov 11, 2024

orzechow Nov 11, 2024 •

edited

Loading

orzechow Nov 11, 2024

orzechow Nov 11, 2024

ll-nick Nov 11, 2024

GitHub action #21

GitHub action #21

Conversation

ll-nick commented Nov 7, 2024 • edited by orzechow Loading

orzechow left a comment

Choose a reason for hiding this comment

orzechow Nov 8, 2024

Choose a reason for hiding this comment

ll-nick commented Nov 8, 2024

orzechow left a comment

Choose a reason for hiding this comment

orzechow commented Nov 8, 2024

ll-nick commented Nov 11, 2024 • edited Loading

ll-nick commented Nov 11, 2024

orzechow left a comment

Choose a reason for hiding this comment

orzechow Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

ll-nick Nov 11, 2024

Choose a reason for hiding this comment

orzechow Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

orzechow Nov 11, 2024

Choose a reason for hiding this comment

orzechow Nov 11, 2024

Choose a reason for hiding this comment

ll-nick Nov 11, 2024

Choose a reason for hiding this comment

ll-nick commented Nov 7, 2024 •

edited by orzechow

Loading

ll-nick commented Nov 11, 2024 •

edited

Loading

orzechow Nov 11, 2024 •

edited

Loading

orzechow Nov 11, 2024 •

edited

Loading