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

build RPM for dom0 in nightly job #129

Merged
merged 11 commits into from
Jan 23, 2020
Merged

build RPM for dom0 in nightly job #129

merged 11 commits into from
Jan 23, 2020

Conversation

redshiftzero
Copy link
Contributor

(reopened from #123 but from the shared remote)

Closes freedomofpress/securedrop-workstation#357

Testing

  1. Check out test build here based on commit ci: commit rpm to test branch for review.
  2. Test rpm is here which you should verify has a bumped version, and has been signed with key with long ID 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:

import rpm


def compare_version(v1, v2):
    """
    Citation: https://github.com/rpm-software-management/yum/blob/1222f377b8dcecb77456ea378a28e1dc23ba4207/rpmUtils/miscutils.py#L36
    Returns:
    * 1 if v1 > v2
    * 0 if v1 == v2
    * -1 if v1 < v2
    """
    return rpm.labelCompare(('0', v1, ''), ('0', v2, ''))


if __name__=='__main__':
    assert compare_version('0.1.1.dev.200116.111111', '0.1.2') == -1
    assert compare_version('0.1.1', '0.1.1') == 0
    assert compare_version('0.1.2.dev.201001.212311', '0.1.2') == 1
    print('all assertions passed')

@redshiftzero redshiftzero added this to Ready for Review in SecureDrop Team Board Jan 18, 2020
@emkll emkll moved this from Ready for Review to Under Review in SecureDrop Team Board Jan 21, 2020
Copy link
Contributor

@emkll emkll left a 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.

  1. Check out test build here : test build/output LGTM
  2. 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.

.circleci/config.yml Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
devops/ci-sign-rpm Show resolved Hide resolved
devops/Dockerfile Show resolved Hide resolved
devops/ci-sign-rpm Outdated Show resolved Hide resolved
devops/ci-sign-rpm Outdated Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
'-' 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.
@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
@redshiftzero
Copy link
Contributor Author

thanks for the detailed comments! I've rebased on latest, this should be ready for re-review

Copy link
Contributor

@emkll emkll left a 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

@emkll emkll merged commit 9a17b8c into master Jan 23, 2020
SecureDrop Team Board automation moved this from Under Review to Done Jan 23, 2020
@emkll emkll deleted the rpm-nightly branch January 23, 2020 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Start creating nightly RPM builds
3 participants