-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Implement knife bootstrap client.d RFC #4529
Conversation
849c82d
to
4608f47
Compare
How does this interact with chef-solo and local mode/knife? Do we want to make a |
def client_d_content | ||
content = "" | ||
if @chef_config[:client_d_dir] && File.exist?(@chef_config[:client_d_dir]) | ||
root = Pathname(@chef_config[:client_d_dir]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a require "pathname"
to this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, good catch
@coderanger good question. My intention was that this does not get loaded with knife as it shouldn't be reading client.rb, and I don't personally see it useful to have for knife. As for solo, I'm open to suggestions. Would people who use solo find it useful? I think solo.d would probably make the most sense for a directory name. |
One use case for |
Interesting. That seems useful. What about solo? |
1b7d6cd
to
29db233
Compare
Just spoke to Jay about this. We agree with Dan that this would be useful for knife users, and the directory for that should be However, my feelings are that we should not do this work for solo. |
The party line is that solo is fully supported, if we do it for client we should do it for solo. |
@coderanger chef-solo being supported doesn't mean guaranteed feature parity. we don't require all features we add to the client mode to also work in chef-solo. Further, RFC031 is accepted as the path to deprecate chef-solo by ensuring local-mode has feature parity and then having chef-solo commands be an alias for local-mode. I don't see any reason to require new feature for chef-solo, only that by supporting it we're promising to not break it. |
@btm Given how little work it would be (the impl is the same as for chef-client), I don't see any reason not to do it other than trying to convince people that they shouldn't use |
@coderanger that's a different argument. If @jaym or whoever implements it looks and thinks the code paths are similar enough that it's easy to add .d support to chef-solo, that's fine, go wild. But it's not required. |
@btm I'm saying it should be required as long as |
29db233
to
6e4e712
Compare
Added conf.d for knife things |
@coderanger then I'm disagreeing. There is no chef-solo based workflow in RFC019 and chef-solo is effectively deprecated except for it's existing UX by RFC031. There's no reason to add additional UX to chef-solo when all it is going to be is a backwards-compatible interface to local-mode, which does support .d configs. |
RFC 19 is both not an exhaustive list and not fully implemented and RFC 31 hasn't been implemented at all. The whole point of RFC 31 is that this would be supported by way of solo being client internally, so why not support it from the start as literally any |
Or alternatively "we don't want to support it", which is fine but it has to be supported until RFC 31 is fully implemented, that was the whole point of RFC 31. |
I'm pretty sure we've never said nor intended that support implies complete feature parity. |
@thommay What does it mean then? Remember that solo is explicitly not deprecated. Keeping a high-quality user experience should mean that non-server-related features should go in both unless there is a compelling reason not to and no one has mentioned a reason other than "we don't want people using chef-solo anymore" which is not a reason I accept. |
I hate the word support. Support means we don't break something. Everything beyond that is trouble. I can't formulate how support relates to new features. We certainly haven't required resource parity across all supported platforms before adding a new resource for one or more platforms. We've added a few features to the client that didn't make it into solo until someone added some kind of compatibility later, e.g. support in solo or via an external cookbook. There's an issue here with our differing development priorities. Let's talk on chef-dev. |
@btm @thommay Solo was actually just broken (existing functionality completely changed) in 12.7 to ensure that its flags had parity with client.
If breaking existing (if rarely or accidentally used) functionality was in order, why not supporting the conf directory? Is the goal for solo to be a debasing experience that encourages switching to client? |
So basically we screwed up, and merged an "improvement" that was actually a net negative for Chef Solo users, including ourselves, who were
Is accidentally deleting a bunch of your files "a debasing experience"? If so, wouldn't making it less likely that you do this on accident be the opposite of that? In any case, this discussion is about adding a feature. |
@luckymike the issue that @danielsdeleo explained aside, the debate is about policy, i.e. by policy do we require feature parity to merge a new feature. It's less about adding the feature this PR represents for solo (I think there's more work/shed-building to figure out the right medium-term UX decisions given RFC031 than there is writing the code for solo.d support). |
I wasn't arguing for deleting home directories, I was trying to offer a clear example of rightfully favoring parity over policy (sem ver). Turning @coderanger's argument for including support in solo into a larger policy debate effectively decides this issue without anyone taking responsibility for it. |
@luckymike by "this issue" you mean #4529, not something related to the This feature is part of the plan I envisioned in the ohai-config RFC discussion for getting ohai UX happy and @coderanger happy about ohai UX being happy. So I'm responsible for it [and @coderanger's happiness!]. But we're in the middle of another hairy chef-client release so it's not my top priority at the moment. |
Yes, the issue #4529, the issue we're commenting on :) |
We can argue about the semantics of the word "deprecated" but I think the reality is that chef-solo is "deprecated". Time spent on chef-solo-specific codepaths is wasted time since chef-zero eventually will be replacing chef-solo via RFC 031. That also completely forces our hand when it comes to conflicting features that have been caused by chef-solo and chef-client/chef-zero having different codepaths. RFC 31 means that at some point in the future the conflicting meanings of the '-r' flag need to be resolved. Generally chef-solo is the smaller userbase and is going to lose that argument. In the case in question its just laughable since making the solo behavior the new meaning of '-r' would mean that everyone using '-r' on the client to mean '--run-list' and doing elastic scaling would have their argument changed to '--recipe-url' and get a complimentary So there isn't parity here. And in fact I think that "solo.d" may buy us a worse compatibility issue since how is that supposed to work when we change chef-solo to invoke chef-client local-mode? Not only is that doing work to create "solo.d" in codepaths that we'll throw away, but we need to have zero pick up solo.d for backcompat when RFC 31 is executed, which is just going to be digging ourselves a deeper hole to climb out of, it doesn't move us any closer towards executing RFC 031. And removal of the rm_rf is entirely because when I was debugging #4368 recently, which is a chef-solo specific issue, I took my usual chef-zero command line argument:
And blindly converted that into a chef-solo command line:
Which immediately wiped out all the cookbooks in my current working directory. Which made me more or less like this: I was actually the one that merged the rm_rf line initially: We've also had chef-solo user requests to remove that line: And not only that but I went and added the "--delete-entire-chef-repo" line and added the rm_rf TO chef-client from chef-solo which didn't exist there before. I just did it in a much safer way where users of "--recipe-url" who do NOT want that behavior do not get it forced down their throat and where a simple command line option like "-r" is not brutally destructive. And I don't see how having to change a command line from:
To:
And getting exactly_the_same_behavior_as_you_had_before is "breaking functionality" or a "debasing experience" |
someone please file an issue about |
13f8b8b
to
b5e6119
Compare
This configuration defaults to config_dir/client.d, but can be set to nil if you do not want chef-client loading any extra config files.
When knife finds a client.d/ directory, it will upload all files nested under that directory.
client.d/*.rb will be read in sorted order. All directories will be ignored.
This will behave similarly to the client.d directory. The top-level ruby files will be loaded in sorted order.
Also, do less mocking so the test will catch the error
Passing the value of `escape_glob` to Dir.glob does not always work correctly on Windows. Always use forward slashes when globbing, even on Windows. We now do this via `escape_glob_dir`, which calls Pathname.cleanpath, which does the foward slash thing.
b5e6119
to
32e7c41
Compare
This makes it the same as application/client.rb. This will let the mixin be used the same way in both places.
32e7c41
to
4cc7325
Compare
@chef/client-maintainers I added solo.d, could probably use another round of reviews |
This commit refactors the spec to use shared examples as the test for client.d and solo.d is the same. knife.d is a little special so it cannot use this.
4cc7325
to
158178c
Compare
|
||
# A directory that contains additional configuration scripts to load for | ||
# the workstation config | ||
default(:conf_d_dir) { PathHelper.join(config_dir, "conf.d") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is supposed to be config.d
because the workstation config is config.rb
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't workstation config usually knife.rb
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
knife.rb
is very softly deprecated. There's no real upside to forcing people to switch, but we decided to use config.rb
going forward because it's used by many tools other than knife.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.rb
is supposed to be used in preference to knife.rb
according to the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL.
👍 with the s/conf_d/config_d/ change. |
confirming my 👍 after the latest updates |
Implement knife bootstrap client.d RFC
This implements the RFC described in chef-boneyard/chef-rfc#183
There are some minor differences:
client.d
is natively interpreted by chef-client in this implementation. It does not rely on a block at the end of the client.rb that loads the files in client.dclient.d
is not found in the bootstrap directory, instead just the repo path.client_d_dir
instead ofclient_d_path
to keep it consistent withtrusted_certs_dir
cc @chef/client-core