Fix for debian/ubuntu hold and a way to add debian src only #333

wilman0 · 2014-07-18T09:25:02Z

modules "apt hold" handling is broken in debian stable cause files in /etc/apt/preferences.d will be silently ignored if files will be split by a blank
add a way to add deb-src to sources.list (useful for build server like jenkins)

daenney · 2014-07-18T09:37:23Z

spec/defines/source_spec.rb

@@ -83,8 +84,9 @@
        if param_hash[:architecture]
          arch = "[arch=#{param_hash[:architecture]}] "
        end
-        content << "\ndeb #{arch}#{param_hash[:location]} #{param_hash[:release]} #{param_hash[:repos]}\n"
-
+        if param_hash [:include_debsrc]


How is this different from include_src?

correct me if im wrong

include_src is just an additional parameter to include a deb + a deb-src line to sources.list. its not possible to include a src line without an deb line using parameter include_src. right?

Sure. But then I don't understand what you're trying to do.

If you want only a deb-src line and no deb line it would have to be named differently, like only_debsrc, check if include_src is actually set to true and on a combination of those conditions only write out a deb-src line and make sure a deb line doesn't exist. Your commit also doesn't seem to do anything with regard to deb-src lines, it writes a deb line.

I also don't understand what your perceive to benefit from only having deb-src lines instead of having both deb and deb-src.

wilman0 · 2014-07-18T12:44:15Z

code updated: include_deb now behave like a switch for "include_deb". the default is include_deb = true

like already mentioned this feature is useful for debian/ubuntu build servers. on a debian wheezy build server you want to include deb-src lines for debian/testing or debian/sid aswell (there is no need for a "debian/testing deb" entry on these servers.

daenney · 2014-07-18T13:01:35Z

Right, now I'm starting to get what you want to do.

daenney · 2014-07-18T13:02:07Z

You've broken a few of the tests:

  1) apt::hold default params creates an apt preferences file
     Failure/Error: })
       expected that the catalogue would contain Apt::Pin[hold vim at 1.1.1]
     # ./spec/defines/hold_spec.rb:33
  2) apt::hold ensure => absent creates an apt preferences file
     Failure/Error: })
       expected that the catalogue would contain Apt::Pin[hold vim at 1.1.1]
     # ./spec/defines/hold_spec.rb:47
  3) apt::hold priority => 990 creates an apt preferences file
     Failure/Error: })
       expected that the catalogue would contain Apt::Pin[hold vim at 1.1.1]
     # ./spec/defines/hold_spec.rb:67
Finished in 28.39 seconds
268 examples, 3 failures

wilman0 · 2014-07-18T13:16:28Z

i think the problem is one of my changes and your tests ;)

according to debian manpage its impossible to set hold for a package by creating a file "/etc/apt/preferenced.d/hold vim at 1.1.1"

all files in path /etc/apt/preferences.d/ delimited by a blank will be silently ignored (give me some time to find the manpage section)

correct and debian valid name would be "hold_packagename" with delimiter "_"

next problem and part of my pull request: please dont create hold files name scheme "hold packagename at versionnumber". we use the pupperlabs-apt module to handle puppet/puppet-common version and to update our puppet version (on all servers)

a change in our puppetlabs-apt manifest to increase the version of puppet results in two different files in /etc/apt/preferences.d

example workflow:

we set apt::hold for package puppet - hold puppet at version 3.3.0
the next puppet run creates /etc/apt/preferences.d/hold_puppet_at_3.3.0
now i increase the version number in my manifest to puppet 3.6.2
instead of rewriting the existing hold file puppetlabs-apt creates a new hold file named "hold_puppet_at_3.6.2" (results in two different hold files for puppet in preferences.d directory)

or is there a way to purge all the files in /etc/apt/preferences.d/ before creating a new hold file?

daenney · 2014-07-18T13:36:30Z

The spec failure is your own mistake, not my tests. You've modified the title of the file passed in by apt::hold into apt::pin. Ergo, the tests in hold_spec that now check for that title fail because they expect the version number in there, which you removed.

My tests were correctly checking the previous behaviour, which is why the build was nice and green. Even though the previous behaviour might be wrong the tests correctly replicated it.

Since you've altered the behaviour, ergo the catalog that's being generated, you need to alter the tests that go with it too.

Until the build goes green this PR won't be considered for merge.

wilman0 · 2014-07-18T15:16:18Z

oh sorry....never used/touched a travis service....thought these checks are only a part of travis-ci/travis configuration.

give me a moment to fix this

daenney · 2014-07-18T16:16:06Z

Oh this is fun, now it's only complaining about Puppet 3. I'll look into that.

wilman0 · 2014-07-18T16:24:28Z

can you give me a hint on this one (for future tests)

if "include_deb" is true deb line will be added to "content" var (line 87)
in line 104 content receive the values of "content"

Failures:

1) apt::source when using default class parameters
Failure/Error: if param_hash [:include_deb]
ArgumentError:
wrong number of arguments (1 for 0)
# ./spec/defines/source_spec.rb:87:in `block (4 levels) in <top (required)>'
# ./spec/defines/source_spec.rb:104:in `block (4 levels) in <top (required)>'

2) apt::source when specifying class parameters
Failure/Error: if param_hash [:include_deb]
ArgumentError:
wrong number of arguments (1 for 0)
# ./spec/defines/source_spec.rb:87:in `block (4 levels) in <top (required)>'
# ./spec/defines/source_spec.rb:104:in `block (4 levels) in <top (required)>'

3) apt::source when specifying class parameters
Failure/Error: if param_hash [:include_deb]
ArgumentError:
wrong number of arguments (1 for 0)
# ./spec/defines/source_spec.rb:87:in `block (4 levels) in <top (required)>'
# ./spec/defines/source_spec.rb:104:in `block (4 levels) in <top (required)>'

...
...
...
Finished in 19.2 seconds
268 examples, 7 failures

wilman0 · 2014-07-29T07:03:29Z

any chance to get this code upstream? do you need more tests...more infos...more code?

with these changes we can switch back to upstream module...

jk779 · 2014-07-29T10:21:52Z

i'd also like to see this fix applied.
having the same problem with ubuntu 12.04 and filesnames with blanks in /etc/apt/preferences.d/

@wilman0 i think it would get merged if travis builds clean. maybe you can have a look at that? :)

lightoze · 2014-07-29T12:36:34Z

Is it possible to move apt::hold fix to a separate pull request? It would be easier to merge it then.

underscorgan · 2014-07-30T19:34:05Z

spec/defines/source_spec.rb

@@ -83,8 +84,9 @@
        if param_hash[:architecture]
          arch = "[arch=#{param_hash[:architecture]}] "
        end
-        content << "\ndeb #{arch}#{param_hash[:location]} #{param_hash[:release]} #{param_hash[:repos]}\n"
-
+        if param_hash [:include_deb]


@wilman0 there's an extra space that's causing unit test failures. Should be
if param_hash[:include_deb]

daenney · 2014-07-30T19:41:22Z

@lightoze in theory I agree with you but it's not really a blocker on the merge, the failing tests are. Thankfully @mhaskel did my job for me so @wilman0 should be able to fix it now and then we can merge.

underscorgan · 2014-07-30T19:44:33Z

Since you added a new parameter @wilman0 for the include_deb it probably requires a documentation update, and if you could squash down to one or two commits that would also be helpful (maybe one commit covering hold, and one commit for include_deb?)

wilman0 · 2014-07-31T11:02:27Z

@mhaskel
thanks for the code fix (passed travis tests)
youre right...to split the changes two different request was my first intention...i gonna change my git workflow to do it next time.

i added 'include_deb' to the readme file. i hope thats enough (i dont think that i should write doku to include src only ... since thats only a corner case for build servers ect)

@everybody
thx for getting this done. it was my first contribution to a public module

...learnings for next time:

git branch to split merge request by feature
bugfixes
- write issue first, afterwards provide patch
new feature
- change documentation
- write tests
- pass tests

underscorgan · 2014-07-31T14:57:38Z

README.md

@@ -203,6 +203,7 @@ Adds an apt source to `/etc/apt/sources.list.d/`.
      key_server        => 'subkeys.pgp.net',
      pin               => '-10',
      include_src       => true
+      include_deb       => true


There should be a comma on the preceding line.

underscorgan · 2014-07-31T15:01:08Z

@wilman0 Other than the typo in the README I think the only other thing we're missing before we'll be able to merge this is a squash. Right now you have 8 commits, but I'd like to see this at one or two commits before merging. Thanks!

wilman0 · 2014-07-31T15:45:25Z

@mhaskel

good idea...can you please tell me how to do it? im a sysop not a dev ;)

do i have to delete the repo...reapply all the changes manually...than create another merge request?

underscorgan · 2014-07-31T15:51:05Z

@wilman0 rebase -i HEAD~9 should get all of your commits, then you can squash everything down from there, and when you push to your remote branch you'll need to do a git push -f.

wilman0 · 2014-07-31T16:25:34Z

@mhaskel
nothing happened. something's missing?

git rebase -i HEAD~9
Successfully rebased and updated refs/heads/master.
git push -f
Everything up-to-date

underscorgan · 2014-07-31T16:30:04Z

@wilman0 ok, when you're rebasing you need to edit all the commits except for the first one to have 'squash' instead of 'pick'. There's a useful section on squashing commits at http://git-scm.com/book/en/Git-Tools-Rewriting-History

fix for default debian installations all files in /etc/apt/preferences without _ will be silently ignore according to debian manpage. Addionally its not a good idea to write versionnumber in filename cause there is no way to delete this files if you increase versionumber Update source_spec.rb add a way to include debsrc only (useful for debian/ubuntu build server ... jenkins ect) Update source_spec.rb var rename Update source.list.erb add include_deb "switch" Update source.pp "include_deb" defaultvalue = true Update hold_spec.rb change the name of the preferences file (hold) Update source_spec.rb Update README.md Doku: 'include_deb' included next to 'include_src' in examples Update README.md typo

wilman0 · 2014-07-31T16:43:26Z

@mhaskel

thx again !!

underscorgan · 2014-07-31T16:58:16Z

@wilman0 thanks for the contribution!

Fix for debian/ubuntu hold and a way to add debian src only

daenney reviewed Jul 18, 2014
View reviewed changes

underscorgan reviewed Jul 30, 2014
View reviewed changes

underscorgan reviewed Jul 31, 2014
View reviewed changes

underscorgan pushed a commit that referenced this pull request Jul 31, 2014

Merge pull request #333 from wilman0/master

e970aa9

Fix for debian/ubuntu hold and a way to add debian src only

underscorgan merged commit e970aa9 into puppetlabs:master Jul 31, 2014

LukasAud added the bugfix label Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for debian/ubuntu hold and a way to add debian src only #333

Fix for debian/ubuntu hold and a way to add debian src only #333

wilman0 commented Jul 18, 2014

daenney Jul 18, 2014

wilman0 Jul 18, 2014

daenney Jul 18, 2014

wilman0 commented Jul 18, 2014

daenney commented Jul 18, 2014

daenney commented Jul 18, 2014

wilman0 commented Jul 18, 2014

daenney commented Jul 18, 2014

wilman0 commented Jul 18, 2014

daenney commented Jul 18, 2014

wilman0 commented Jul 18, 2014

wilman0 commented Jul 29, 2014

jk779 commented Jul 29, 2014

lightoze commented Jul 29, 2014

underscorgan Jul 30, 2014

daenney commented Jul 30, 2014

underscorgan commented Jul 30, 2014

wilman0 commented Jul 31, 2014

underscorgan Jul 31, 2014

underscorgan commented Jul 31, 2014

wilman0 commented Jul 31, 2014

underscorgan commented Jul 31, 2014

wilman0 commented Jul 31, 2014

underscorgan commented Jul 31, 2014

wilman0 commented Jul 31, 2014

underscorgan commented Jul 31, 2014

Fix for debian/ubuntu hold and a way to add debian src only #333

Fix for debian/ubuntu hold and a way to add debian src only #333

Conversation

wilman0 commented Jul 18, 2014

daenney Jul 18, 2014

Choose a reason for hiding this comment

wilman0 Jul 18, 2014

Choose a reason for hiding this comment

daenney Jul 18, 2014

Choose a reason for hiding this comment

wilman0 commented Jul 18, 2014

daenney commented Jul 18, 2014

daenney commented Jul 18, 2014

wilman0 commented Jul 18, 2014

daenney commented Jul 18, 2014

wilman0 commented Jul 18, 2014

daenney commented Jul 18, 2014

wilman0 commented Jul 18, 2014

wilman0 commented Jul 29, 2014

jk779 commented Jul 29, 2014

lightoze commented Jul 29, 2014

underscorgan Jul 30, 2014

Choose a reason for hiding this comment

daenney commented Jul 30, 2014

underscorgan commented Jul 30, 2014

wilman0 commented Jul 31, 2014

underscorgan Jul 31, 2014

Choose a reason for hiding this comment

underscorgan commented Jul 31, 2014

wilman0 commented Jul 31, 2014

underscorgan commented Jul 31, 2014

wilman0 commented Jul 31, 2014

underscorgan commented Jul 31, 2014

wilman0 commented Jul 31, 2014

underscorgan commented Jul 31, 2014