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

Fix dependency on apt-transport-https #1110

Merged
merged 1 commit into from
Aug 11, 2017
Merged

Conversation

rvdh
Copy link
Contributor

@rvdh rvdh commented Aug 11, 2017

Without this change, the nginx https repository url is added to sources.list.d
and apt-get update is executed immediately after, failing because
apt-transport-https is not yet installed. Subsequently all package
installs fail because the apt cache has been invalidated.

With this change we ensure the apt-transport-https package is present
before running apt-get update. Since all sources are now https, the
package is always necessary, which is why I moved it out of the 'case'
statement.

Without this change, the nginx https repository url is added to sources.list.d
and apt-get update is executed immediately after, failing because
apt-transport-https is not yet installed. Subsequently all package
installs fail because the apt cache has been invalidated.

With this change we ensure the apt-transport-https package is present
before running apt-get update. Since all sources are now https, the
package is always necessary, which is why I moved it out of the 'case'
statement.
@wyardley wyardley merged commit 2234e7d into voxpupuli:master Aug 11, 2017
@wyardley
Copy link
Collaborator

This seems sensible to me, and we can probably close #1014, apologies to the author of that one.

@wyardley wyardley self-requested a review August 11, 2017 22:06
@oxc
Copy link

oxc commented Dec 10, 2017

I'm afraid the update of puppetlabs-apt to 4.4.0 (https://forge.puppet.com/puppetlabs/apt/4.4.0/changelog) breaks compatibility with this change, I'm getting the following dependency cycle:

Error: Found 1 dependency cycle:
(Package[apt-transport-https] => Apt::Source[nginx] => Package[apt-transport-https])

Does this mean the dependency should be removed again?

@ekohl
Copy link
Member

ekohl commented Dec 10, 2017

I do think it was incompatible before but you're right: we should remove all off the apt https code and rely on puppetlabs-apt 4.4.0 to do it right. @oxc would you be willing to submit a PR?

@oxc
Copy link

oxc commented Dec 10, 2017

@ekohl unfortunately I'm not totally sure what to remove, and how to test if it does not break anything. Especially the passenger-specific require parameter puzzles me a bit.

@ekohl
Copy link
Member

ekohl commented Dec 10, 2017

If you could test #1163 that'd be great.

@oxc
Copy link

oxc commented Dec 10, 2017

Turns out I get the cycle even without manage_repos, so I won't be able to test your fix yet (because ther error even occurs without the explicit dependencies on apt-transport-https)

Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Fix dependency on apt-transport-https
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants