search

chef / omnibus-ctl

Provides service control for omnibus packages
Apache License 2.0
23 stars 24 forks source link

License acceptance during ctl reconfigure #42

Closed sersut closed 8 years ago

sersut commented 8 years ago

This PR adds license checking functionality to ctl reconfigure command. At a high level this PR:

  1. Makes ctl reconfigure display the LICENSE to the user and asks for acceptance if a license exists for a project.
  2. Enables --accept-license CLI option to be used in scripts.

More details about this feature can be found here

I have been investigating which component is the best to implement this functionality. I have thought about using:

  1. enterprise-chef-common cookbook which is shared across most of our projects.
  2. Adding a new cookbook called chef-license which contains this logic.

The main issue I ran into was to implement --accept-license functionality. reconfigure() runs Chef and the only way to parameterize the chef run is to use --json-attributes option which is already being used with file "#{base_path}/embedded/cookbooks/dna.json" in reconfigure.

I'm open to suggestions of implementing this in a different place. And yes this change will be a breaking change and we are working on a communication plan here.

Please LMK of your thoughts and please include others who would be interested in what's happening here :smile: If all is good I will finalize the PR with more tests.

/cc: @nellshamrell, @stevendanna, @marcparadise, @smith, @tylercloke, @sdelano, @schisamo, @patrick-wright

sersut commented 8 years ago

@stevendanna I added a commit per your comments. LMK what you think.

stevendanna commented 8 years ago

@sersut My only high-level question is whether it would be better to do this at package install time?

I'm not thrilled with the idea of chef-server users hitting this on upgrade, but I gather that this is more or less a requirement.

sersut commented 8 years ago

We have considered doing something during the postinst but didn't want to go that path since it will be platform specific and much more fragile.

Even during that path though, users will hit this on upgrade no?

stevendanna commented 8 years ago

Even during that path though, users will hit this on upgrade no?

Yup.

sersut commented 8 years ago

:white_check_mark: http://wilson.ci.chef.co/job/chef-server-12-test/179/

sersut commented 8 years ago

@stevendanna having moved Chef Server out of the way, is there anything else needed for this PR?

Anyone else needs to :eyes: at this?

marcparadise commented 8 years ago

:+1:

mmzyk commented 8 years ago

Information on the various ways to accept the license should probably be added to any --help output as well, so that it is easier for someone to find that information.

tylercloke commented 8 years ago

👍 tech wise. I think we should think really carefully around the UX of this though. This has the potential to be a painful user experience. I'd feel a lot better about this if we demoed this to our UX people first.

mmzyk commented 8 years ago

Looks good to me @sersut. Thanks for all the work here.