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

MODULES-10543: remove sources.list file on purging #904

Merged
merged 1 commit into from
Feb 13, 2020

Conversation

anarcat
Copy link

@anarcat anarcat commented Feb 11, 2020

The current sources.list purge behavior is inconsistent with the
preferences behavior. The former creates a file with a comment, the
latter removes the file.

This harmonizes this behavior between the two files and simply drops
the file if it is to be purged.

The current sources.list purge behavior is inconsistent with the
preferences behavior. The former creates a file with a comment, the
latter removes the file.

This harmonizes this behavior between the two files and simply drops
the file if it is to be purged.
@anarcat anarcat requested a review from a team as a code owner February 11, 2020 16:48
@anarcat
Copy link
Author

anarcat commented Feb 11, 2020

tests have failed but that's not because of this patch, at least i hope not:

https://travis-ci.org/puppetlabs/puppetlabs-apt/jobs/649004533

the error is:

     #   Error: Execution of '/usr/bin/apt-key adv --no-tty --keyserver pool.sks-keyservers.net --recv-keys 6F6B15509CF8E59E6E469F327F438280EF8D349F' returned 2: Warning: apt-key output should not be parsed (stdout is not a terminal)

     #   Executing: /tmp/apt-key-gpghome.0K6LpO0RLF/gpg.1.sh --no-tty --keyserver pool.sks-keyservers.net --recv-keys 6F6B15509CF8E59E6E469F327F438280EF8D349F

     #   gpg: keyserver receive failed: Cannot assign requested address

i would argue strongly against using keyservers, as they do stuff like that, but what do i know... ;)

@baldurmen
Copy link

This makes a lot of sense to me. Thanks @anarcat for that patch!

@anarcat anarcat closed this Feb 11, 2020
@anarcat anarcat reopened this Feb 11, 2020
@codecov
Copy link

codecov bot commented Feb 11, 2020

Codecov Report

Merging #904 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #904   +/-   ##
=======================================
  Coverage   73.07%   73.07%           
=======================================
  Files           5        5           
  Lines         260      260           
=======================================
  Hits          190      190           
  Misses         70       70

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 d5884ab...1859c94. Read the comment docs.

@anarcat
Copy link
Author

anarcat commented Feb 11, 2020

trying to restart travis again again.

@anarcat anarcat closed this Feb 11, 2020
@anarcat anarcat reopened this Feb 11, 2020
@anarcat
Copy link
Author

anarcat commented Feb 11, 2020

tests have failed but that's not because of this patch, at least i hope not:

tests are now passing. i opened a bug report in JIRA about that problem as well:

https://tickets.puppetlabs.com/browse/MODULES-10544

@carabasdaniel
Copy link

Hi @anarcat

Thank you for your contribution, this look great.

@carabasdaniel carabasdaniel merged commit a2af17f into puppetlabs:master Feb 13, 2020
@anarcat anarcat deleted the sources-list-remove branch February 13, 2020 13:17
@petak-it
Copy link

petak-it commented Apr 6, 2020

Hi @anarcat ,
i found a pretty major issue with this change on removing sources.list file completely. On Ubuntu system there is automatic creation of sources.list file. I could not find what's doing that (think that apt system when upgrade). But this causing that every day (looks like it's created once a day) to puppet schedule removing that file. Another issue was with basic Ubuntu package command-not-found that using sources.list file and causing error when triggered by miss-typed command.

Thanks

@anarcat
Copy link
Author

anarcat commented Apr 6, 2020

hi @petak-it

i think if you have something competing with Puppet to recreate the file, you have two options:

  1. fix that thing which competes with puppet
  2. tell puppet to stop purging the file

My patch did not force purging the file. It just changed the effect of the $purge[sources.list] parameter to be consistent with the $purge[preferences] parameter. If you set it to false, it will not touch your sources.list file.

Furthermore, if command-not-found relies only on sources.list (and not sources.list.d), it's buggy and should be fixed.

Finally, if you see a bug in this module, the right way to report it is not in a comment in a pull request. It's in JIRA, at https://tickets.puppetlabs.com/

I hope that helps

@pebtron
Copy link

pebtron commented May 20, 2020

Hi @anarcat ,
i found a pretty major issue with this change on removing sources.list file completely. On Ubuntu system there is automatic creation of sources.list file. I could not find what's doing that (think that apt system when upgrade). But this causing that every day (looks like it's created once a day) to puppet schedule removing that file. Another issue was with basic Ubuntu package command-not-found that using sources.list file and causing error when triggered by miss-typed command.

Thanks

I can confirm that after upgrading to this latest release of puppetlabs-apt that we are seeing Puppet run these twice per day on our Ubuntu 18.04 LTS fleet:

2020-05-20 17:03:20 -0500 /Stage[main]/Apt/File[sources.list]/ensure (notice): removed (corrective)
2020-05-20 17:03:26 -0500 /Stage[main]/Apt::Update/Exec[apt_update] (notice): Triggered 'refresh' from 1 event

This post sheds light on the handful of Ubuntu services that are recreating /etc/apt/sources.list as an empty zero byte file, and the post also illustrates the headache involved in disabling these services:
https://unix.stackexchange.com/questions/315502/how-to-disable-apt-daily-service-on-ubuntu-cloud-vm-image

I would like to propose that the intent of 'purge' with regards to sources.list is akin to "purging any Apt sources out of sources.list" rather than deleting the file from the system. If we modify this module so that 'purge' doesn't delete the sources.list file but instead ensures that it's zero bytes, then it will play nice with the various Ubuntu tools that rely on this file existing while still "purging any Apt sources out of sources.list". I would be happy to code this and submit a Pull Request.

@sheenaajay
Copy link

We will revert the changes and get a new PR for this feature in which we will keep the previous default behaviour of creating an empty file while purging.
If the user wants to go head and delete the file , a parameter will be added to do force purge.
Currently its causing regression to our customers and would like to revert the changes in the PR and implement the change as suggested above (https://tickets.puppetlabs.com/browse/MODULES-10804). Thanks alot for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants