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

Clean up PR #180 which consolidates the chef provisioners and uses the Omnitruck API to fetch the version #200

Merged
merged 7 commits into from
Jan 6, 2020

Conversation

arizvisa
Copy link
Contributor

@arizvisa arizvisa commented Jan 6, 2020

This PR is based on PR #180 and supersedes it by splitting up the single-commit into multiple commits for each change to the related files. The core intention of PR #180 is to use Chef's Omnitruck API for determining the CHEF_URL. This allows us to not have to do any work to determine which URL to download Chef's installers from. It also appears that the logic for installing all the Chef-related things is the same, so this PR uses two cases for handling the Chef provisioning in in scripts/cmtool.bat.

The first branch is :omnitruck. This label determines all the components needed to query the Omnitruck API to find the .msi that needs to be installed. After the request is made, the URL is extracted from the metadata that was returned, and then the CHEF_URL variable is assigned with the result. At this point we can simply branch to the :chef label.

The original branch, :chef, was dumbed down to simply download the contents of CHEF_URL and then kick off the silent installer. We should never enter this case without executing :omnitruck first as we need someone to explicitly assign the CHEF_URL to download+install our target.

Once I finish testing this, I'll remove the WIP label.

…cmtool.bat to explicitly specify a lowercase OMNITRUCK_MACHINE_ARCH.
@arizvisa
Copy link
Contributor Author

arizvisa commented Jan 6, 2020

  • chef
  • chefdk
  • chef-workstation

@arizvisa arizvisa changed the title [WIP] Clean up PR #180 which consolidates the chef provisioners and uses the Omnitruck API to fetch the version Clean up PR #180 which consolidates the chef provisioners and uses the Omnitruck API to fetch the version Jan 6, 2020
@arizvisa
Copy link
Contributor Author

arizvisa commented Jan 6, 2020

Was able to build the latest of each (chef, chefdk, chef-workstation) successfully with the eval-win7x64-enterprise.json template, and the latest of chef with the eval-win7x86-enterprise.json template.

@arizvisa arizvisa added the enhancement This will introduce or improve an already existing capability label Jan 6, 2020
@arizvisa arizvisa merged commit cd25232 into boxcutter:master Jan 6, 2020
@daxgames
Copy link
Contributor

daxgames commented Jan 6, 2020

@arizvisa I like the changes, however, they may break anyone trying to use Chef to manage config while building the image unless they have built in the license acceptance into their custom build scripts. The stable Chef channel requires a license acceptance for any Chef component to be used. You need to be below a specific version to avoid the license acceptance.

Edit: Maybe setting CM_VERSION to unlicensed sets the version for each product to the highest version that does not require an acceptance and leave the default as latest. Just a thought.

Edit: Removed backward compatibility issue. I missed line 47 setting omnitruck_version to cm_version. I initially viewed the changes from my phone.

@arizvisa
Copy link
Contributor Author

arizvisa commented Jan 6, 2020

Does this PR break what #180 was trying to accomplish, and it need to be reverted/worked-on? Or are you saying just that PR #180 is just not the correct way to install chef?

@daxgames
Copy link
Contributor

daxgames commented Jan 7, 2020

@arizvisa In spirit it still does what PR #180 was trying to do and I like what you did but Chef changed its licensing model and the newer versions of the Chef products require a license to be accepted upon use or to pre-configured as accepted.

You changed the code to default to 'latest' in omlitruck which will require any use of Chef to be pre-approved or any automation using Chef will fail.

PR #180 was written before this license model change, while it defaulted to latest, latest was a hardcoded CM_VERSION value in the cmtool script that did not require a license acceptance. Not sure you saw my edited comment above but I think the edits are pretty self explanatory.

To be backward compatible and not break existing users while considering the license model change I see a few of options:

Option 1 - Return to previous functionality and restore backward compatibility.

  1. If CM_VERSION is unset it defaults to latest
    • Setting CM_VERSION to the highest version that does not require an acceptance.
    • Use omnitruck to install the specified version
  2. If CM_VERSION is set to something other than latest use omnitruck to install the specified version
  3. Creating an image where if Chef is used and CM_VERSION requires acceptance it would need to be automated into the instance startup.

Option 2 - Enhancement 1

  1. A new setting for Chef called CHEF_LICENSE_ACCEPTANCE that defaults to false
  2. If CM_VERSION is unset it defaults to latest
    • Setting CM_VERSION to the highest version that does not require and acceptance.
  3. If CM_VERSION is set use omnitruck to install the specified version
  4. Leave the license pre-acceptance unconfigured.
  5. Creating an image where if Chef is used and CM_VERSION requires acceptance it would need to be automated into the instance startup.

Option 3 - Enhancement 2

  1. If CHEF_LICENSE_ACCEPTANCE is set to true
  2. If CM_VERSION is unset it defaults to latest use omnitruck to discover and install the latest version.
  3. If CM_VERSION is set to latest use omnitruck to discover and install the latest version.
  4. If CM_VERSION is set use omnitruck to install the specified version
  5. Configure license pre-acceptance.
  6. Creating an image where if Chef is used and CM_VERSION requires acceptance it is pre-configured.

@arizvisa
Copy link
Contributor Author

arizvisa commented Jan 7, 2020

Ok. Sorry for merging PR #180 then, I didn't know that it would break things because none of the side effects were discussed in any of the issue trackers or in the PR itself. If PR #200 is different from the scope of #180 was trying to solve, then it should be unmerged.

I was just intending to push these PRs along because I've had a number of PRs sitting in the queue that have appeared abandoned, and the worst thing as a developer is submitting code to a project and waiting 3 years for it to get merged.

@arizvisa
Copy link
Contributor Author

arizvisa commented Jan 7, 2020

Nonetheless, if you want to update your PR to fix your concerns, feel free to submit it and I'll merge it in your changes.

@daxgames
Copy link
Contributor

daxgames commented Jan 7, 2020

Its not a big deal. I will look at it as in in master.

@daxgames
Copy link
Contributor

daxgames commented Jan 7, 2020

I am testing with the following added code:

+if not defined CM_LICENSED set CM_LICENSED=false
+
+if "%CM_VERSION%" == "latest" if "%CM_LICENSED%" == "false" (
+    echo ==^> Setting %OMNITRUCK_PRODUCT% version to specific unlicensed supported value.
+    if "%OMNITRUCK_PRODUCT%" == "chef-workstation" (
+        set CM_VERSION=0.3.2
+    ) else if "%OMNITRUCK_PRODUCT%" == "chefdk" (
+        set CM_VERSION=3.12.10
+    ) else if "%OMNITRUCK_PRODUCT%" == "chef" (
+        set CM_VERSION=14.14.29
+    )
+)

 if not defined OMNITRUCK_VERSION set OMNITRUCK_VERSION=%CM_VERSION%

This will implement 'Option 2 - Enhancement 1'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This will introduce or improve an already existing capability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants