-
Notifications
You must be signed in to change notification settings - Fork 12
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
CI: Build debian packages and run autopackage tests #130
base: main
Are you sure you want to change the base?
Conversation
1a4eecc
to
aef2d42
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #130 +/- ##
==========================================
+ Coverage 86.87% 87.03% +0.16%
==========================================
Files 64 64
Lines 4838 4891 +53
==========================================
+ Hits 4203 4257 +54
+ Misses 444 441 -3
- Partials 191 193 +2 ☔ View full report in Codecov by Sentry. |
e92325b
to
f2aa14a
Compare
9df4ea2
to
7281cd3
Compare
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.
There is a lot of assumptions on that PR, in particular in which distro to runs. Also, the containers in polluted with preparing the build package and end up having network connection when building it, which is not what a real ppa or builder is doing.
I think it will be better to reuse https://github.com/canonical/desktop-engineering/blob/main/gh-actions/common/build-debian/action.yml, amend it for anything needed without extending too much complexity. Please check with @EduardGomezEscandell who is the primary author.
Some of the changes that can be made for instance it to not rely on
- name: Build package
uses: jtdor/build-deb-action@v1
For instance, which should be easy enough to do.
That way, we can decide on which releases to build and ensure we have something closer to a real build environement. Adding autopkgtests as a separate action is interesting there too.
.github/workflows/build-deb.yaml
Outdated
- name: Install dependencies | ||
run: | | ||
apt install equivs devscripts lintian | ||
mk-build-deps -ir |
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 is an old way of instlaling build deps, let’s not create the dummy package are really on build-dep .
funcitonality of apt.
.github/workflows/build-deb.yaml
Outdated
push: | ||
branches: | ||
- main | ||
tags: |
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.
We don’t want to restrict on tags, everything that is push to main or opened as PR should have that passing.
Consider this was done as a quick thing to test #120 (and actually caught few issues during the final testing of that PR :)). So, there are assumptions but all depending on variables, so ideally can be multiplexed to multiple jobs, although this was not a requirement yet.
Yeah, this has to be changed indeed.
However I was looking at the code of that action and I'm not sure how internet is prevented there too, since it seems that certificates are still installed. However I'll dig on that more further. |
Hi, thanks for taking a look :)
The action works in two phases, each in a separate container.
|
Something I don't like of that is that it's running the build process as root, which is what other builders do, but it should not be the case. I can probably patch that to fix this though. |
I've tried again using
It's quite weird since it does seems to run in the same environment.... And funny thing: there's no relevant difference in source packages as per So.... I'm not really sure what it is. I'll check in future, but I think that for now the safest way would just go with this option (once I address the comments on building based on the source and disabling internet access), instead of continuing living without a CI action that ensures we're still packaging authd properly. |
96257cf
to
dd5f794
Compare
Opening this again, even though now it's using canonical/desktop-engineering#28 |
Ensure that autopkgtests works with authd at every run
dd5f794
to
0a23351
Compare
name: Build debian packages | ||
|
||
on: | ||
push: | ||
branches: | ||
- main | ||
pull_request: | ||
|
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'm not a big fan of having this running on every single PR and push to main, tbh. I think it's better to trigger this only if there were any changes on debian/
, otherwise it will increase our CI time to run even more for little gain.
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.
Eh... I would agree on that, if it wouldn't that test may fail in different conditions, and having it tested in a properly isolated testbed is the way.
I think also @didrocks agree'd as per #130 (comment).
However, one thing we could do is limit to changes on **/**_test.go
file maybe?
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.
Not sure if there's a way in github to run only a workflow when a PR is approved? As that would be a way to prevent problems to hit to main without having to test all the PR's from the very first moment.
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.
You can add a job that relies on the completion of another one by setting the needs:
field, but I still think that this is not something we need to constantly watch for.
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.
Also, I'm afraid this kind of action might give users the wrong impression that the code in main
is always "ready to use" when, in reality, this kind of project relies a lot on manual tests to ensure none of our dependencies regressed.
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.
No, well that's not the point IMHO but at the same it's useful for testing having packages ready all the time.
But well, if someone think that, then is wrong as it would be wrong in any other project doing nightly builds.
However, I can see if can change this to reduce the scope of its action. I guess we'll do this after FF though since this still depends on canonical/desktop-engineering#28
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 is a CI related PR, so it can definitely wait and be done after FF.
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.
Yeah, the reason for that was to get this early to be confident on uploading things working stuff to the distro, but we can still append these commits to temporary branches to check things for now.
…s and add tests (#28) As underlined in ubuntu/authd#130 (comment) using `jtdor/build-deb-action` for building debian packages has some problems that are yet not clear, and it also does not seem to properly support building non-native packages. As per this, given that the code needed for handling the build inside a simpler docker container is quite small, I've moved the build to manual handling which allows us some more control over the steps at the cost of being a bit more verbose. As bonus: - We now only build as user (not the sources, since there's no much point) - All the network traffic is totally blocked for the builder user (not just HTTPS) - ~~`DEB_BUILD_OPTIONS` can be customized for special builds (e.g. `nocheck`)~~ - Version uses actual distro version I've added some commits from #22, but not included the lintian build yet (that I've ready [here](3v1n0@1b72205)) not to make this PR harder to check, but also because I think those could instead be part of another action to allow more parallelization when used. By doing this change, not only both test cases pass, but also authd is correctly built: ubuntu/authd#130 (e.g. https://github.com/ubuntu/authd/actions/runs/8044158864) UDENG-2439
Add a workflow to build debian packages and run autopkgtests.
UDENG-2343