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

Adding Prefix and Group tags #59

Merged
merged 6 commits into from
Jul 15, 2019
Merged

Conversation

AdrianWennberg
Copy link
Contributor

Prefix and Group tags added for RPMs only since those tags do not exist for deb packages.

Also defined some macros for rpmbuild to make nfpm less dependent on the build environment.

Our build system has a complex rmpbuld environment set up where
macros don't match with the defaults that nfpm exepcts. This
change will enforce those defaults which makes nfpm less
dependant on the build environment.
These tags are only implemented for RPMs because there is
no good parallel to draw to deb tags.

See goreleaser#57
@caarlos0
Copy link
Member

any thing like this for deb?

if its only for RPM, maybe prefix the config with RPM or inside a RPM block to make it clearer 🤔

@AdrianWennberg
Copy link
Contributor Author

AdrianWennberg commented Jul 10, 2019

The Section tag which is already implemented for deb-only seems to be similar to the Group tag for RPMs, but it does not make sense to use one tag for both since they have different naming conventions.

For reference:
RPM packages by group: https://www.rpmfind.net/linux/RPM/Groups.html
Deb packages by section: https://packages.debian.org/stable/

Putting the new tags into an RPM block seems like a good solution to avoid confusion.

@caarlos0
Copy link
Member

Yeah seems good to me! And the deb specific ones we can put under a Deb block too..

willing to make this change?

@AdrianWennberg
Copy link
Contributor Author

Moving the Section tag under a Deb block might break current config files if people update nfpm without updating their config. Do you think this will be an issue for people?

Other that that, I'd be happy to make the change!

@AdrianWennberg
Copy link
Contributor Author

Oops forgot to actually create the package in the test.

@codecov-io
Copy link

codecov-io commented Jul 10, 2019

Codecov Report

Merging #59 into master will increase coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
+ Coverage   71.57%   71.68%   +0.11%     
==========================================
  Files           5        5              
  Lines         496      498       +2     
==========================================
+ Hits          355      357       +2     
  Misses         87       87              
  Partials       54       54
Impacted Files Coverage Δ
nfpm.go 89.65% <ø> (ø) ⬆️
rpm/rpm.go 73.41% <ø> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7939798...e6c4e32. Read the comment docs.

@AdrianWennberg
Copy link
Contributor Author

Hey Carlos,

I think this is merge ready now. Give me a shout if there is anything else you would like me to add.

Group: "default",
Prefix: "/usr",
}
var err = Default.Package(info, ioutil.Discard)
Copy link
Member

Choose a reason for hiding this comment

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

maybe write to a file and compare with the expected result? (like goldenfiles above)

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 added a new commit. Does that look good to you?

Copy link
Member

Choose a reason for hiding this comment

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

looks great, thanks

@caarlos0 caarlos0 merged commit 76d7d4f into goreleaser:master Jul 15, 2019
@caarlos0
Copy link
Member

thank you very much for the PR and all the effort :) ✨

@caarlos0
Copy link
Member

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@goreleaser goreleaser locked as resolved and limited conversation to collaborators Nov 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants