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

Relocate License Logic on Windows systems into Ruby #212

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

Conversation

jwdean
Copy link
Contributor

@jwdean jwdean commented Aug 31, 2020

Previous logic used PowerShell to call ruby.exe, with the ruby being an embedded string within the PowerShell script. That ruby used LicenseAcceptance::Acceptor to apply the license acceptance condition. This also used chef_install_dir to determine where Chef was located.

The new logic eliminates the embedded Ruby by determining if there is license acceptance before starting PowerShell to modify the command line of the post-action ChefClient execution. This now will use the --chef-license command line parameter to affect the license state. Instead of using chef_install_dir to determine where Chef is installed it will use the exec_command resource.

Description

There are two desired impacts of this change

  1. Stylistically it is poor form to embed Ruby in PowerShell that is effectively called by Ruby. If there were tests for this code, it would be untestable.

  2. The use of chef_install_dir is used for two non-congruent purposes in the cookbook:
    -- determines the directory where the PowerShell script to be run is located.

    Kernel.spawn("c:/windows/system32/schtasks.exe /F /RU SYSTEM /create /sc once /ST \"#{upgrade_start_time}\" /tn Chef_upgrade /tr \"powershell.exe -ExecutionPolicy Bypass \"#{chef_install_dir}\"/../chef_upgrade.ps1 2>&1 > #{chef_upgrade_log}\"")

    That location would (by default) be C:\OpsCode.

    -- determines where ruby.exe is located (in PowerShell).

    #{chef_install_dir}/embedded/bin/ruby.exe -e "

    The need this addresses is to allow the PowerShell script to be run from a directory other than that in which Chef is installed. This is a security requirement for us. While the more obvious change may have been to modify the behavior of the first case above, the, changing the second is a light touch that does should not change the current, expected behavior.

Issues Resolved

None

Check List

@jwdean jwdean requested a review from a team August 31, 2020 19:54
----------

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the
    best of my knowledge, is covered under an appropriate open
    source license and I have the right under that license to
    submit that work with modifications, whether created in whole
    or in part by me, under the same open source license (unless
    I am permitted to submit under a different license), as
    Indicated in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including
    all personal information I submit with it, including my
    sign-off) is maintained indefinitely and may be redistributed
    consistent with this project or the open source license(s)
    involved.

Signed-off-by: Jeff Dean <jwdean@scriptpro.com>
Copy link

@phiggins phiggins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good change to me, but I can't tell if the CI failures are related to this PR.

@thsteven13
Copy link

TravisCI seems to be failing on this repo for about a year now with the same error. So i dont think it's related to this PR.

Comment on lines 501 to 507
post_action = if (new_resource.post_install_action == 'exec') && (desired_version.major >= 15)
"#{new_resource.exec_command} --chef-license #{license_provided}"
elsif desired_version.major < '15'
new_resource.exec_command
else
''
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
post_action = if (new_resource.post_install_action == 'exec') && (desired_version.major >= 15)
"#{new_resource.exec_command} --chef-license #{license_provided}"
elsif desired_version.major < '15'
new_resource.exec_command
else
''
end
post_action = if new_resource.post_install_action != 'exec'
''
elsif desired_version.major >= 15
"#{new_resource.exec_command} --chef-license #{license_provided}"
else
new_resource.exec_command
end

I think this conditional needs to change to maintain the old behavior.

The whitespace is probably all wrong because I'm trying to do this in github's comment box.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Thank you.

----------

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the
    best of my knowledge, is covered under an appropriate open
    source license and I have the right under that license to
    submit that work with modifications, whether created in whole
    or in part by me, under the same open source license (unless
    I am permitted to submit under a different license), as
    Indicated in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including
    all personal information I submit with it, including my
    sign-off) is maintained indefinitely and may be redistributed
    consistent with this project or the open source license(s)
    involved.

Signed-off-by: Jeff Dean <jwdean@scriptpro.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants