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

Package and publish an RPM file as well as debian #49

Merged
merged 5 commits into from
Nov 4, 2020
Merged

Conversation

orenbm
Copy link
Contributor

@orenbm orenbm commented Nov 3, 2020

Resolves conjurinc/AAM#319

We would like to offer the packages also to Linux-based machines that run RPM. Thus, we need to package and publish the components as RPM files in addition to debians.

Once this PR is merged, projects that will run debify will publish an RPM file to our redhat repo in artifactory.

An example can be seen in this build of evoke that uses the debify docker image created by this side-branch.

Note: Every run of debify will package and publish also an RPM package. While this is needed for conjur-ui, possum and evoke, it is not needed for conjur-ldap-sync as that RPM will not be used in the dockerless image. So the artifcatory repo will have unused packages of conjur-ldap-sync.

@orenbm orenbm self-assigned this Nov 3, 2020
@orenbm orenbm force-pushed the package-rpm branch 3 times, most recently from f0c2e21 to 5e3c41b Compare November 3, 2020 10:08
We would like to offer the packages also to Linux-based
machines that run RPM
@orenbm orenbm marked this pull request as ready for review November 3, 2020 13:17
@orenbm orenbm requested a review from a team November 3, 2020 13:19
Copy link

@rafis3 rafis3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orenbm looks very good. Added one question and two comments.

Also one general comment: I think it would be good to add something small in the README of the repo that explains that now Debify doesn't just create deb packages. The name used to be self explanatory but now it's not anymore, so further explanation could help.

Given I successfully run `env DEBUG=true GLI_DEBUG=true debify package -d ../../example -v 0.0.1 example -- --post-install /distrib/postinstall.sh`
# We use version 0.0.1-suffix to verify that RPM converts dashes to underscores
# in the version as we expect
Given I successfully run `env DEBUG=true GLI_DEBUG=true debify package -d ../../example -v 0.0.1-suffix example -- --post-install /distrib/postinstall.sh`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the adding of suffix needed for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please read the comment i wrote above :)

Conjur::Debify::Utils.copy_from_container container, "/src/#{package_name}"
puts "#{package_name}"
begin
Conjur::Debify::Utils.copy_from_container container, "/dev-pkg/#{dev_package_name}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's critical, but what do you think about this suggestion:
The logic of copy_packages_from_container could perform a copy of a single package. You will have four calls to this function instead of two. The calls will be for rpm dev, rpm, deb dev, deb. The calls for deb dev and rpm dev, will be wrapped with the rescue statement. This way the function is more generic, it just copies packages name from a given location. The logic on how to handle the copy is up to the caller, that knows what type of package it copies and if a failure is accepted or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think that this will simplify the code. the current one is good and will be called once for each file type. i don't expect that we'll need the extra generic level and adding it will add redundant complexity.

-t $file_type \
-n conjur-$project_name-dev \
-v $version \
-C . \
--maintainer "Conjur Inc." \
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's worth changing Conjur Inc. to CyberArk? Don't want to increase your scope, only if you feel comfortable doing that.
Same with the url, from https://www.conjur.net to https://www.cyberark.com

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what should we do with this?

  spec.authors       = ["Kevin Gilpin"]
  spec.email         = ["[email protected]"]

@izgeri do you have a suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof let's definitely update this

authors: CyberArk Software, Inc.
email: [email protected]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks :)

fpm -s dir -t deb -n conjur-$project_name -v $version -C . \
--maintainer "Conjur Inc." \
--vendor "Conjur Inc." \
for file_type in deb rpm
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this appear twice? line 70 and again in 33?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once for the dev package and one for the prod. this logic was here before my change.

README.md Outdated
@@ -1,5 +1,10 @@
# Debify

Debify is a tool used for building and testing Conjur appliance packages.
Copy link
Contributor

@jvanderhoof jvanderhoof Nov 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Debify is a tool used for building and testing Conjur appliance packages.
Debify is a tool used for building and testing DAP appliance packages.

Semantics, but I'd love to see us start to remove "Conjur" when talking about DAP components.

The description now explains that alongside the debian file we also
create an RPM one.
@orenbm
Copy link
Contributor Author

orenbm commented Nov 4, 2020

@jvanderhoof can you please re-review? Are you reviewing the PR on behalf of core-team or would you like someone else to do that?

Copy link
Contributor

@jvanderhoof jvanderhoof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this looks good. It's not a show stopper, but any updates to the Readme providing some overview of how to use the feature would be super valuable.

@orenbm
Copy link
Contributor Author

orenbm commented Nov 4, 2020

Overall I think this looks good. It's not a show stopper, but any updates to the Readme providing some overview of how to use the feature would be super valuable.

there's no real usage really. When you run the project it will now package and publish an RPM package alongside the debian one. So the usage hasn't changed, just the impact.

@orenbm orenbm merged commit 1f30888 into master Nov 4, 2020
@orenbm orenbm deleted the package-rpm branch November 4, 2020 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants