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

feat: enforce packager defaults #372

Merged

Conversation

drewstinnett
Copy link
Contributor

Not completely addressing #100, but giving it a first go!

This PR adds in some deb specific defaults, specifically around:

  • Although the docs are a little murky (some say this field must be set, others say it is just recommended), there are some 3rd party tools that will barf if priority isn't set, so it feels like a good idea to set this to a safe default on all debs
  • Maintainer is a 'mandatory' field...leaving it unset still results in a usable deb package, however installing the deb package prints a nasty warning about not having a maintainer

Would love any feedback/suggestions around making this better.

One thing I'm a little torn on is setting the Maintainer field to a bogus value. Even though this field is marked as mandatory in some of the deb documentation, but I would hate to break existing setups by making nfpm for real require it, instead of just setting a bogus value.

When (or if 🤣) you're cool with the implementation details, I'll be happy to extend this to rpm/apk too. Thanks!!

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 8, 2021
@vercel
Copy link

vercel bot commented Oct 8, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/goreleaser/nfpm/5Xqapkk4Eqeb2RXewToANHxxQH61
✅ Preview: https://nfpm-git-fork-drewstinnett-feature-enforce-pa-4aa66a-goreleaser.vercel.app

@vercel vercel bot temporarily deployed to Preview October 8, 2021 00:25 Inactive
@erikgeiser
Copy link
Member

My preferred way would be this: Set a bogus value but print out a "deprecation" warning. Don't document the bogus value but document that leaving is empty is "deprecated". After a while (or with v3 if necessary) we remove the bogus value and force the maintainer to be set.

Maybe another word would fit this purpose better than "deprecation", though.

What do you you think?

@drewstinnett
Copy link
Contributor Author

@erikgeiser , that would work from my end! I will update the PR

@erikgeiser
Copy link
Member

A safe value for the priority would be optional I guess. According to the docs: "This is the default priority for the majority of the archive. Unless a package should be installed by default on standard Debian systems, it should have a priority of optional. Packages with a priority of optional may conflict with each other."

Oh by the way, maybe the bogus value should be Unset Maintainer <unset@localhost>?

@vercel vercel bot temporarily deployed to Preview October 8, 2021 16:40 Inactive
deb/deb.go Outdated
@@ -167,7 +167,8 @@ func (*Deb) SetPackagerDefaults(info *nfpm.Info) {
// if in the long run we should be more strict about this and error when
// not set?
if info.Maintainer == "" {
info.Maintainer = "unset maintainer unset@example.com"
log.Println("DEPRECATION WARNING: Unset 'maintainer' field on deb packages is deprecated and will be removed in a future version")
Copy link
Member

@erikgeiser erikgeiser Oct 8, 2021

Choose a reason for hiding this comment

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

That kind of makes it sound like the maintainer field will be removed. What about DEPRECATION WARNING: Leaving the 'maintainer' field unset will not be allowed in a future version. Same in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good call, adjusting now. Ty for the thoughtful feedback on this!

@vercel vercel bot temporarily deployed to Preview October 8, 2021 16:57 Inactive
@caarlos0 caarlos0 changed the title Feature: enforce packager defaults feat: enforce packager defaults Oct 11, 2021
Copy link
Member

@caarlos0 caarlos0 left a comment

Choose a reason for hiding this comment

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

LGTM!

thanks!

@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #372 (ccdd639) into master (df14bee) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
+ Coverage   65.03%   65.19%   +0.16%     
==========================================
  Files          14       14              
  Lines        1284     1290       +6     
==========================================
+ Hits          835      841       +6     
  Misses        313      313              
  Partials      136      136              
Impacted Files Coverage Δ
deb/deb.go 67.17% <100.00%> (+0.60%) ⬆️

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 df14bee...ccdd639. Read the comment docs.

@caarlos0 caarlos0 merged commit 56c46c6 into goreleaser:master Oct 11, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants