Skip to content
This repository has been archived by the owner on Jan 19, 2019. It is now read-only.

Add apt-get update if compile_time is true, COOK-4534 #32

Closed
wants to merge 1 commit into from
Closed

Add apt-get update if compile_time is true, COOK-4534 #32

wants to merge 1 commit into from

Conversation

hhoover
Copy link

@hhoover hhoover commented Apr 10, 2014

No description provided.

@brint
Copy link

brint commented Apr 10, 2014

This will help with cloud providers 👍

@andygrunwald
Copy link

+1. Runs without problems.
Thanks, this fixed a issue with a debian deployment.

@sethvargo
Copy link
Contributor

👎. I removed this from the build-essential cookbook in the latest refactor. I feel this is a huge anti-pattern as it is the responsibility of the cookbook user to ensure the system is in a viable state. The fact that the apt cache is out of date is out of scope for this (and any) cookbook except the apt cookbook.

This seems like logic that should either be added to the apt cookbook or done in part of a wrapper cookbook.

@ryandub
Copy link

ryandub commented May 8, 2014

@sethvargo While it may be true that the apt cookbook and other cookbooks should deal with the apt cache, if build-essential is running at compiletime (which is required for many things now-a-days) it happens before the apt cookbook runs and so the run fails. @hhoover 's request fixes this contention.

I don't understand your point that "it is the responsibility of the cookbook user to ensure the system is in a viable state". Most (all?) cloud providers have images that come online with stale apt caches. Does it make sense to modify workflow to have to manually update the cache before running Chef? Users are accustomed already to not having to do this and having build-essential take care of this for them - what is the advantage of taking this away?

Is there a better method to deal with this that we haven't thought of?

@sethvargo
Copy link
Contributor

@ryandub what you're advocating for is a tight coupling between a package manager and a cookbook. That's a bad idea. This is an apt-specific problem and I firmly believe it is outside the scope of this cookbook to manage the apt cache.

I'd be open to a conversation about adding the ability for the apt cookbook to refresh it's cache at compile time, but having the build-essential cookbook update the apt-cache seems like an anti-pattern to me. That's like having the Jenkins cookbook install nginx/apache - it's all about correctly scoping the job of the cookbook. Otherwise you end up with largely unmaintainable and untestable codebases with hundreds of edge cases.

If the apt cookbook had the ability to update it's cache at compile time, would that solve your problem(s)?

@hhoover
Copy link
Author

hhoover commented May 8, 2014

The main issue is if build-essential and apt are both set to run at compile time, build-essential wins and runs first, regardless of run_list order. Since that is the case, having build-essential update the apt cache makes sense. If apt ran first (or Chef respected the run_list) this would not be a problem.

It's working with this PR and not just for me, but also @ryandub and @andygrunwald. Merging this fixes this issue. There's no code to merge to fix Chef's disrespect of the run_list today.

@ryandub
Copy link

ryandub commented May 8, 2014

Thanks @sethvargo - the Apt cookbook has an attribute for compiletime, but it did not work as expected. Looking through the code, this appears to only add the cacher-client proxy stuff at compile time and not update the cache itself. I think this caused some confusion (it did with me at least) in the past.

If the apt cookbook is patched to update cache at compile time BEFORE build-essential runs at compile time this would fix the issue. I will work on a patch for apt to do this.

However, in the future, I think it would be beneficial to the community for impacted cookbooks (at least well known ones like apt) to be updated at the same time as a breaking change like this in order to preserve functionality.

@andygrunwald
Copy link

Thanks for taking care and to add so much love and effort @hhoover and @ryandub!

@sethvargo
Copy link
Contributor

@hhoover @ryandub @andygrunwald okay. I'm going to close this PR then. If we are all in agreement that this logic belongs in the apt cookbook, let's put it in there. Feel free to /cc me on the PR and I'll take a look. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

5 participants