Add apt_repository resource #4782
Conversation
thommay
force-pushed
the
tm/apt_repository
branch
from
18e9a98
to
d197b59
Compare
| end | ||
|
|
||
| action :add do | ||
| converge_by "add apt repository for #{new_resource.name}" do |
There was a problem hiding this comment.
This means this resource will always be marked as updated which doesn't seem right?
thommay
force-pushed
the
tm/apt_repository
branch
5 times, most recently
from
b2c06ed
to
da230d2
Compare
|
@chef/client-core ready for review… |
| resource_name :apt_repository | ||
| provides :apt_repository | ||
|
|
||
| property :repo_name, String, name_property: true, regex: [/^([a-z]|[A-Z]|[0-9]|_|-|\.)+$/] |
There was a problem hiding this comment.
this smells a bit like another brittle regex?
|
looks like you addressed sous-chefs/apt#191 ? |
| end | ||
|
|
||
| def extract_fingerprints_from_cmd(cmd) | ||
| so = Mixlib::ShellOut.new(cmd, env: { "LANG" => "en_US", "LANGUAGE" => "en_US" }) |
There was a problem hiding this comment.
not entirely clear why you're going to Mixlib::ShellOut here instead of shell_out()?
|
it should fix sous-chefs/apt#191 ; sous-chefs/apt#192 ; sous-chefs/apt#200 at least. But I don't want to actually close them until i port this code back to the cookbook. |
|
just a couple of questions, but 👍 overall |
thommay
force-pushed
the
tm/apt_repository
branch
from
da230d2
to
d390610
Compare
| end | ||
|
|
||
| def extract_fingerprints_from_cmd(cmd) | ||
| so = shell_out(cmd, env: { "LANG" => "en_US", "LANGUAGE" => "en_US" }) |
There was a problem hiding this comment.
i think you can actually drop the lang stuff since this code should have you completely covered:
https://github.com/chef/chef/blob/master/chef-config/lib/chef-config/config.rb#L872-L922
including handling the case of servers that have no en_US.utf-8 or C.utf-8 support and falls back to C and yells at the user:
https://github.com/chef/chef/blob/master/chef-config/lib/chef-config/config.rb#L911
More or less stolen from the apt cookbook, but tweaked and fixes a heap of bugs.
thommay
force-pushed
the
tm/apt_repository
branch
from
d390610
to
79548ac
Compare
|
|
||
| property :repo_name, String, name_property: true | ||
| property :uri, String | ||
| property :distribution, String, default: lazy { node["lsb"]["codename"] } |
There was a problem hiding this comment.
I love that this was added to core Chef, but...
@thommay Having this as a default is not a good idea because some Apt repos do not use a distribution and having this here makes it impossible to use those things. Also, it is a different behavior than the existing apt cookbook, which causes it to break backwards compatibility.
There was a problem hiding this comment.
i belive this property is nillable so its not impossible?
(i admit i can't recall where we wound up with nillable properties in chef-12... its a bit difficult to keep straight...)
There was a problem hiding this comment.
fixing here: #4832
🚧 needs more tests, but basic review appreciated 🚧