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

WIP: feat: Use conventional file names by default #157

Conversation

erikgeiser
Copy link
Member

This pull request changes the behaviour of the --target CLI option to produce conventional file for the respective packages by default. Conventional file names are for example package_version_architecture.package-type for Debian packages and name-version-release.architecture.rpm for RPMs. Consider the following cases:

  • --target is blank or omitted: Create a package with a conventional file name in current directory
  • --target is a directory: Create a package with a conventional file name in the directory
  • --target is a file name: Use this file name

Why?

Because at the moment, nfpm allows users to omit --target and uses the very weird default /tmp/foo.deb in this case. A default value should cover the most common use cases and creating the file /tmp/foo.deb would be a very weird use case and is just plain wrong for RPM files.

At the moment, there are a few issues:

  • I don't know if adding a new function to the Packager interface is a bit too drastic, but currently, I cannot move the function to nfpm.go (where I initially put it) because it needs access to the architecture translation and it has to be ensured that every future architecture provides such a name.
  • I also changed a bit at the end of doPackage() because the success message now has to be printed there. At the same time, I changed the behaviour to remove the empty package file if packaging fails. Is it okay to also include that in this pull request?
  • I could not run the tests because I don't have access to docker on this machine

@vercel
Copy link

vercel bot commented Jul 9, 2020

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/hrdk1607s
✅ Preview: https://nfpm-git-fork-erikgeiser-feature-conventional-e993f9.goreleaser.vercel.app

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 9, 2020
@vercel vercel bot temporarily deployed to Preview July 9, 2020 11:38 Inactive
@erikgeiser erikgeiser force-pushed the feature/conventional-file-names-by-default branch from a8fd6b4 to 999c9d3 Compare July 9, 2020 12:04
@vercel vercel bot temporarily deployed to Preview July 9, 2020 12:04 Inactive
@caarlos0
Copy link
Member

caarlos0 commented Jul 9, 2020

  • I don't know if adding a new function to the Packager interface is a bit too drastic, but currently, I cannot move the function to nfpm.go (where I initially put it) because it needs access to the architecture translation and it has to be ensured that every future architecture provides such a name.

IMHO its OK.

  • I also changed a bit at the end of doPackage() because the success message now has to be printed there. At the same time, I changed the behaviour to remove the empty package file if packaging fails. Is it okay to also include that in this pull request?

I think we can do it in another one, since this one is pretty much done...

  • I could not run the tests because I don't have access to docker on this machine

no problem, CI passed :) although there are some missing coverage, but we can handle that later..

@erikgeiser
Copy link
Member Author

I think we can do it in another one, since this one is pretty much done...

This is already included in this pull request because I had to change this part due to the call to fmt.Printf after pkg.Package(). So now we have these options:

A: Before this pull request

	return pkg.Package(info, f)

B: Currently in this pull request

	err = pkg.Package(info, f)
	_ = f.Close()
	if err != nil {
		os.Remove(target)
		return err
	}

	fmt.Printf("created package: %s\n", target)
	return nil

C: If I remove the file removal again:

	err = pkg.Package(info, f)
	if err != nil {
		return err
	}

	fmt.Printf("created package: %s\n", target)
	return nil

What do you prefer for this pull request?

@caarlos0
Copy link
Member

caarlos0 commented Jul 9, 2020

Let's leave as is then.

Can I merge it?

@erikgeiser
Copy link
Member Author

Can I merge it?

As far as I'm concerned, sure.

@caarlos0 caarlos0 merged commit 4804199 into goreleaser:master Jul 9, 2020
@caarlos0
Copy link
Member

caarlos0 commented Jul 9, 2020

awesome, thanks for the PR :)

@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
enhancement New feature or request 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.

None yet

3 participants