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

Add an apt_update resource #4422

Merged
merged 6 commits into from
Jan 27, 2016
Merged

Add an apt_update resource #4422

merged 6 commits into from
Jan 27, 2016

Conversation

thommay
Copy link
Contributor

@thommay thommay commented Jan 18, 2016

No description provided.

@lamont-granquist
Copy link
Contributor

probably needs to have the apt-get-update-periodic stuff baked into it to be really useful...

@ranjib
Copy link
Contributor

ranjib commented Jan 18, 2016

what is the intended use for this? currently resources like apt_repository uses something similar internally.
LGTM

@thommay
Copy link
Contributor Author

thommay commented Jan 18, 2016

@lamont-granquist yeah, although I wanted to throw something up to get the shape right. I think a fair amount of time the flow is going to be:

  • add apt repository
  • notify apt_update immediately

but I guess the other half is the apt_update right at the start of the run...

@ranjib I think it sucks that you need the apt cookbook to do an apt-get update sensibly. And the chef upgrade work is going to want to be able to do this, too.

@lamont-granquist
Copy link
Contributor

i think the intended use is to kill off the apt cookbook dependency, so that cookbooks can just do something more like:

apt_update if node['platform_family'] == 'debian'

and then nobody has to sync down the apt cookbook, and rhel/windows/solaris users can stop bitching.

we can also resolve the entirely silly situation where the apt cookbook is a hard dependency of practically every cookbook out there, but because we don't want to piss off the rhel users, we don't make it a hard dependency, which means that every cookbook is broken unless users know to manually add the apt repo (and requires the apt cookbook recipe being added in every kitchen.yml out there for testing on ubuntu).

@coderanger
Copy link
Contributor

👎 in current form due to the excessive syncs it would run. We would need to port over the timestamp checking at a minimum. 👍 if we do that, though it only avoids the apt dependency in the simplest of cases (i.e. not using apt_repository), but that is probably enough of them to be worthwhile.

@lamont-granquist
Copy link
Contributor

I think there's an (approved?) RFC to slurp up apt_repository into core chef as well?

@thommay
Copy link
Contributor Author

thommay commented Jan 19, 2016

@chef/client-ubuntu this is ready for review...

@lamont-granquist I don't think that ever got written. But we should.

@lamont-granquist
Copy link
Contributor

ah, it got a bunch of thumbs, but then deciderated that submissions into core chef didn't require an RFC for some reason rather than just being accepted... chef-boneyard/chef-rfc#101

@coderanger
Copy link
Contributor

👍

true
else
false
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could just return the conditional here.

also i thought there was more machinery involved in getting apt-get update to update the periodic stamp?

@thommay
Copy link
Contributor Author

thommay commented Jan 21, 2016

@chef/client-debian @chef/client-ubuntu this is really ready for review now, please.

[STAMP_DIR, APT_CONF_DIR].each do |d|
build_resource(:directory, d, caller[0]) do
recursive true
end.run_action(:create)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't have use_inline_resources here so you should still have to manually do the updated_by_last_action? dance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, i see, its always in a converge_by block and always sets the resources as being updated... nevermind, move along then....

@danielsdeleo
Copy link
Contributor

👍

1 similar comment
@lamont-granquist
Copy link
Contributor

👍

thommay added a commit that referenced this pull request Jan 27, 2016
@thommay thommay merged commit b7e7f42 into master Jan 27, 2016
@lamont-granquist lamont-granquist deleted the tm/apt_update branch February 17, 2016 22:10
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants