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

For Debian packages, register files in `/etc` as conf files #877

Merged
merged 1 commit into from Apr 24, 2015

Conversation

Projects
None yet
2 participants
@vincentbernat
Contributor

vincentbernat commented Mar 18, 2015

When building Debian packages with debhelper, files in /etc are
automatically marked as configuration files (this is done since
compatibility level 3 which was introduced in 2000). Therefore, packages
built with fpm may come as a surprise since they don't do that. A user
can still add --config-files /etc but they have to know that (usually
after a valuable modification has been erased).

This change add /etc to the list of configuration files for Debian
unless fpm is invoked with --no-config-files.

Closes: #873

Unlike what I was told in #873, I didn't add any warning for the behavior change. How should I add that?

@jordansissel

This comment has been minimized.

Owner

jordansissel commented Apr 10, 2015

@vincentbernat I think we can have a logger.warn when we are activating the default within fpm, basically telling the user something like "Debian packaging policy generally labels all files in /etc as config files, so fpm defaults to this behavior for deb packages. You can disable this default behavior with --no-config-files flag"

@@ -117,6 +117,9 @@ def help(*args)
"configuration files, specify this flag multiple times. If argument is " \
"directory all files inside it will be recursively marked as config files.",
:multivalued => true, :attribute_name => :config_files
option "--no-config-files", :flag,

This comment has been minimized.

@jordansissel

jordansissel Apr 10, 2015

Owner

Given this is a debian-specific patch, I'm have hesitations on having it listed as a default flag and not deb-specific. Thoughts?

This comment has been minimized.

@vincentbernat

vincentbernat Apr 10, 2015

Contributor

At first, I thought that it would be the same for RPM, but all spec files seem to have to declare the files as a configuration file (with %config). So, I will change the option to be Debian specific.

allconfigs << p.gsub("#{staging_path}/", '') if File.file? p
end
rescue Errno::ENOENT => e
add_path path, allconfigs

This comment has been minimized.

@jordansissel

jordansissel Apr 10, 2015

Owner

use parentheses - add_path(path, allconfigs)

# Strip leading /
path = path[1..-1] if path[0,1] == "/"
cfg_path = File.expand_path(path, staging_path)
Find.find(cfg_path) do |p|
allconfigs << p.gsub("#{staging_path}/", '') if File.file? p

This comment has been minimized.

@jordansissel

jordansissel Apr 10, 2015

Owner

Since Find.find returns an Enumerator, I think moving the conditional File.file? to a select would make it easier to read:

Find.find(cfg_path).select { |path| File.file?(path) }.each do |path|
  allconfigs << path.gsub("#{staging_path}/", "")
end
config_files.each do |path|
# expand recursively a given path to be put in allconfigs
def add_path(path, allconfigs)

This comment has been minimized.

@jordansissel

jordansissel Apr 10, 2015

Owner

I'm not sure how I feel about passing the allconfigs in for the purposes of modification, hmm... Seems like this would be better moved into a separate class where you do configs.add_path(path) instead.

I'm OK if your code stays as-is, for now, while I ponder this :)

# Also add everything in /etc
begin
add_path '/etc', allconfigs unless attributes[:no_config_files?]

This comment has been minimized.

@jordansissel

jordansissel Apr 10, 2015

Owner

We should log that we are doing this, as you asked about the warning about the behavior change, maybe somethign like:

# Written assuming '--no-config-files' stays as the flag name
if !attributes[:no_config_files?]
  logger.warn("Something about what we are doing, and why....")
  add_path("/etc", allconfigs)
end
@@ -366,7 +366,6 @@ def dpkg_field(field)
@deb.attributes[:deb_user] = "root"
@deb.attributes[:deb_group] = "root"
@deb.instance_variable_set(:@config_files, ["/etc/init.d/test"])

This comment has been minimized.

@jordansissel
@jordansissel

This comment has been minimized.

Owner

jordansissel commented Apr 10, 2015

Thank you for helping fpm build better things by default :)

I did one pass at reviewing the code. Overall, it looks great! I made some comments on naming and style and some other things.

@vincentbernat vincentbernat force-pushed the vincentbernat:fix/debian-etc-in-conffiles branch from df2cb8c to 139ab4e Apr 10, 2015

@vincentbernat

This comment has been minimized.

Contributor

vincentbernat commented Apr 10, 2015

I have updated the PR with the requested changes: use of --deb-no-config-files, various coding style. I left the add_path method as is. Using a separate class may be a bit overkill. There is a spurious commit that I have also pushed into a separate PR (#890) to make unittests work.

@jordansissel

This comment has been minimized.

Owner

jordansissel commented Apr 13, 2015

@vincentbernat looks good!

One last question, I hope! I was thinking since this is specific to /etc and "automatic" or "default" behavior, maybe the flag should be --no-default-config-files ? -no-config-files seems too blunt of a phrasing when we mean something specific.

Otherwise, LGTM!

@vincentbernat vincentbernat force-pushed the vincentbernat:fix/debian-etc-in-conffiles branch from 139ab4e to 3e23e46 Apr 15, 2015

@vincentbernat

This comment has been minimized.

Contributor

vincentbernat commented Apr 15, 2015

Just updated the PR with --no-default-config-files. This makes sense.

For Debian packages, register files in `/etc` as conf files
When building Debian packages with debhelper, files in `/etc` are
automatically marked as configuration files (this is done since
compatibility level 3 which was introduced in 2000). Therefore, packages
built with fpm may come as a surprise since they don't do that. A user
can still add `--config-files /etc` but they have to know that (usually
after a valuable modification has been erased).

This change add `/etc` to the list of configuration files for Debian
unless fpm is invoked with `--deb-no-config-files`.

Closes: #873

@vincentbernat vincentbernat force-pushed the vincentbernat:fix/debian-etc-in-conffiles branch from 3e23e46 to 71f7ea2 Apr 15, 2015

@jordansissel jordansissel force-pushed the jordansissel:master branch 2 times, most recently from 715ab62 to 9866c6d Apr 24, 2015

@jordansissel

This comment has been minimized.

Owner

jordansissel commented Apr 24, 2015

LGTM

jordansissel added a commit that referenced this pull request Apr 24, 2015

Merge pull request #877 from vincentbernat/fix/debian-etc-in-conffiles
For Debian packages, register files in `/etc` as conf files

@jordansissel jordansissel merged commit f2bdc2f into jordansissel:master Apr 24, 2015

jordansissel added a commit that referenced this pull request Jun 20, 2016

Merge pull request #877 from vincentbernat/fix/debian-etc-in-conffiles
For Debian packages, register files in `/etc` as conf files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment