-
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
build RPM for dom0 in nightly job #129
Conversation
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.
Reviewed this PR by first going through the test plan as described in the PR description, it all looks great to me.
- Check out test build here : test build/output LGTM
- Test rpm is here: Artifact looks good to me, and is correctly signed with the securedrop test key
Some minor comments inline, mostly for discussion. Other than adding a couple of comments for maintainability, this is good to merge from my perspective.
'-' is considered an illegal character in the version, so we're going to go with LATEST_TAG.YYMMDD.HHMMSS
also the changelog absense doesn't fail the build, so ignoring
Need to specify the image for the machine executor else it defaults to trusty git-lfs package is not available in xenial according to https://packages.ubuntu.com/xenial/git-lfs
we can either modify the rpmbuilder image in the containers image (though this signing process required additional packages and sudo), or we can leave this as is. Since it's running nightly, the additional time of the docker image build is not going to be a major annoyance.
and fix the repo name
@emkll pointed out during review that if we ran these steps on a per-PR job, we'd have a higher chance of catching breakage in the rpm CI build logic prior to merge
* specify key by fingerprint instead of uid * remove `| true` since gpg --import does return 0 if the key is already imported * add note where this FEDORA_PKGR_VER came from
bfc36a3
to
fb18e58
Compare
thanks for the detailed comments! I've rebased on latest, this should be ready for re-review |
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.
Thanks @redshiftzero, changes look good to me, confirmed the latest changes are working in https://circleci.com/gh/freedomofpress/securedrop-debian-packaging/2420
(reopened from #123 but from the shared remote)
Closes freedomofpress/securedrop-workstation#357
Testing
ci: commit rpm to test branch for review
.0x4A3BE4A92211B03C
Note: if you notice issues with the files in the rpm, and notice something is missing, please file those over in securedrop-workstation. The next step here will be actually using this rpm in freedomofpress/securedrop-workstation#406
Versioning
I had to deviate from our versioning strategy used for the debian package nightlies as
-
is an invalid character, so I went with LATEST_TAG.dev.YYMMDD.HHMMSS which is as close as possible for consistency. I verified via this script which you can run in a Fedora AppVM that this versioning is what we want: