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 a dist option to support community branches #691

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

patcable
Copy link
Contributor

@patcable patcable commented Jul 6, 2020

This makes it a bit easier to use community forks with Chef.

Description

Community forks may use a different dist name, but have the same basic setup. To that end, this adds a dist attribute that lets people use different community distributions of Chef with this cookbook. The config namespace is unchanged, as interpolation there may prove to be a bit unreadable.

Issues Resolved

As is, if you override these variables (say you're using cinc):

  node.override['ohai']['plugin_path'] = '/etc/cinc/ohai/plugins'
  node.override['chef_client']['conf_dir'] = '/etc/cinc'
  node.override['chef_client']['bin'] = '/opt/cinc/bin/cinc-client'
  node.override['chef_client']['log_dir'] = '/var/log/cinc'
  node.override['chef_client']['run_path'] = '/var/run/cinc'
  node.override['chef_client']['file_cache_path'] = '/var/cache/cinc'
  node.override['chef_client']['file_backup_path'] = '/var/lib/cinc'

you can deploy using this cookbook - however the run mechanisms (ie. systemd on ubuntu) will still be put down on disk as chef-client.service which may be confusing for folks.

Check List

@patcable
Copy link
Contributor Author

patcable commented Jul 6, 2020

👋 I'm not sure if this something folks would be interested in, but, it is a thing that seems to work in my environment. I didn't add tests because reasonably the existing ones should pass as i imagine the default would be kept to chef. Happy to take suggestions on things that could be refactored or redone, or if this isn't something y'all are interested in that's OK as well.

@patcable
Copy link
Contributor Author

patcable commented Jul 7, 2020

Hmmm... so, this is pretty half baked now that i've run into a few issues, mainly: i was a little optimistic with the use of node.override in my wrapper. Overrides aside, let me know how i can refactor this and add a test in for the dist flag.

@patcable
Copy link
Contributor Author

I removed the non-working interpolation and rebased; though being new to tests i'm a little unsure of how to proceed there.

@patcable patcable marked this pull request as ready for review July 15, 2020 22:59
@phiggins
Copy link

Hi, thanks for your contribution. 👋

  • There's a few CI failures that should be straightforward to deal with. First, you'll need to signoff your commits for the DCO check. There's also some lint failures that you should be able to clear up by running cookstyle -a to autofix them.
  • I don't have a lot of experience with cookbooks, but I know you can access Chef helper code in them. Can you access the Chef::Dist constant here to access the distribution specific strings that are already setup? That would probably be a much more maintainable way to enable this change.

Signed-off-by: Patrick Cable <[email protected]>
Signed-off-by: Patrick Cable <[email protected]>
Signed-off-by: Patrick Cable <[email protected]>
@patcable
Copy link
Contributor Author

Hey @phiggins - makes sense re: Chef::Dist though i'm a little unclear on how to pull it and maintain backwards compatibility. Could something like this work? Went back to find the first version it appeared in, which, presumably is the first version a fork would be most realistic anyways?

I guess the next part is testing. I'll poke around some of that.

@bcg62
Copy link
Contributor

bcg62 commented Sep 30, 2020

would also find this useful. it would make transitioning easier.

@phiggins
Copy link

phiggins commented Oct 1, 2020

Hi,

First I'd like to apologize, I dropped the ball on this one and only saw your response yesterday because of the new comment.

I brought this up with the rest of the team and the consensus was that while this change does make logical sense, it would add too much of a maintenance burden to make it worthwhile. I'm not sure of other cookbooks have handled this but it might be worth investigating that before spending a lot more time on this.

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

Successfully merging this pull request may close these issues.

3 participants