Expose notify_update to apt::source

[danielhoherd]

This solved a dependency cycle for us.

[daenney]

This is not true. You're using undef as the default which, by virtue of apt::setting have it set to true by default means it turns out to be true. Your test however isn't testing the default chain, it's explicitly setting it as true.

You should have two test cases for this. One not passing in anything and validating that it will notify and the second explicitly setting it to true and checking that it propagates.

[danielhoherd]

Good catch. Fixed.

[daenney]

Please squash this down to a single commit with a descriptive title and body.

[daenney]

This solved a dependency cycle for us.

Could you elaborate? Before we add any feature we'd like an example of use case so we can estimate if this is a feature worth adding or if it's a "0.1% solved and issue for me" thing.

[danielhoherd]

@daenney Here is the error that this patch solved:

Error: Could not apply complete catalog: Found 1 dependency cycle:
(Exec[apt_update] => Exec[silent_apt_update] => Package[apt-transport-s3] => File[/etc/apt/s3auth.conf] => Apt::Source[aptly_aptly_s3] => Apt::Setting[list-aptly_aptly_s3] => File[/etc/apt/sources.list.d/aptly_aptly_s3.list] => Apt::Setting[list-aptly_aptly_s3] => Apt::Source[aptly_aptly_s3] => Apt::Source[aptly_ubuntu_s3] => Apt::Setting[list-aptly_ubuntu_s3] => File[/etc/apt/sources.list.d/aptly_ubuntu_s3.list] => Exec[apt_update])

By using this patch, we can install apt-transport-s3 without causing a cycle. We use this so we can add the s3 transport, then place some s3 apt::source files, then do the update. Doing an update prior to placing the s3 transport and s3 apt::source files is irrelevant and could potentially cause problems.

We were unable to find another way to prevent triggering an update when defining an apt::source, which is why we made this patch. If there is another simple method to do this then I'd love to hear it.

Not having notify_update exposed seemed like a feature omission to me, or at least an inconsistency. This flag was already added to apt::conf and apt::pin in 9ad4fd6

[josephholsten]

would this allow us to remove /~https://github.com/puppetlabs/puppetlabs-apt#adding-new-sources-or-ppas? If so, I believe this is behaviour that 99.9% of users want.

Also, this no longer needs the needs-squash label.

[daenney]

Not having notify_update exposed seemed like a feature omission to me, or at least an inconsistency. This flag was already added to apt::conf and apt::pin in 9ad4fd6

The reason for not exposing it was because no one has needed it before. This is how things go. We can't build 100% of all features anyone ever wants since sometimes that's not a good idea and other times we've simply not hit that use case/problem before. Even though one resource has the flag exposed it doesn't necessarily make sense to expose that same flag on everything else.

[daenney]

@tphoney @hunner @bmjen I see no real issues w.r.t merging this. I think it makes sense though I doubt it resolves the problem @josephholsten mentioned.

[tphoney]

Ok @daenney i will merge this. It does look like we need a separate PR to address the issue raised by @josephholsten