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.

Sign up for GitHub

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

Jump to bottom

[MSYS-726] Allow setting environment variables at the user level #6612

Merged

thommay merged 7 commits into chef:master from MsysTechnologiesllc:harry/allow_setting_environment_variables_at_the_user

Feb 12, 2018

harikesh-kolekar commented Dec 1, 2017 •

edited

Description

The env resource allows you to set a system environment variable, but windows also allow per-user environment variables.

https://blogs.technet.microsoft.com/heyscriptingguy/2005/03/18/how-can-i-create-an-environment-variable-using-a-script/

It looks like we could simply add a user property and pass it in obj.username in the existing provider instead of "".

Acceptance criteria:

User property for env resource

Check List

New functionality includes tests
All tests pass
RELEASE_NOTES.md, has been updated if required (not required for bugfixes, required for API changes)
All commits have been signed-off for the Developer Certificate of Origin. See https://github.com/chef/chef/blob/master/CONTRIBUTING.md#developer-certification-of-origin-dco

harikesh-kolekar requested a review from a team

December 1, 2017 09:38

harikesh-kolekar force-pushed the harry/allow_setting_environment_variables_at_the_user branch 3 times, most recently from edab568 to 42ee06a Compare

December 1, 2017 12:26

Contributor

coderanger commented Dec 1, 2017

This is not part of the abstract interface for "global env vars" so we are probably at (really: long past, but whatevs) the point that this resource needs to be renamed windows_path or this property should be moved into a platform-specific resource :-/ If we want to keep it in the generic one, we should make sure all other providers raise an exception if it is used so it doesn't silently Do The Wrong Thing.

harikesh-kolekar force-pushed the harry/allow_setting_environment_variables_at_the_user branch 2 times, most recently from 8506445 to 6d77c3a Compare

December 5, 2017 12:09

Contributor

btm commented Dec 5, 2017 •

edited

this property should be moved into a platform-specific resource :-/ If we want to keep it in the generic one, we should make sure all other providers raise an exception if it is used so it doesn't silently Do The Wrong Thing.

@coderanger What would a 'platform-specific resource' be? Just renaming it windows_env? The resource is currently limited to the windows platform here and thus we do raise exceptions elsewhere, e.g.:

2017-12-05T12:08:08-05:00] FATAL: Chef::Exceptions::NoSuchResourceType: Cannot find a resource for env on mac_os_x version 10.11.6

Contributor

coderanger commented Dec 5, 2017

Yes, windows_env would be a better name if this is going to be a specific model for Windows environment variables.

harikesh-kolekar force-pushed the harry/allow_setting_environment_variables_at_the_user branch 3 times, most recently from 41f013a to e31e3c3 Compare

December 11, 2017 10:11

brewn mentioned this pull request

[MSYS-736]Allow setting environment variables at the user level chef/chef-web-docs#1151

Closed

harikesh-kolekar force-pushed the harry/allow_setting_environment_variables_at_the_user branch from e31e3c3 to 696746f Compare

December 13, 2017 09:12

btm approved these changes

View reviewed changes

Contributor

btm left a comment

thanks @harikesh-kolekar

@tas50, did you look into deprecating env => windows_env ? We should probably do that as a separate change anyway since I think it'll take some time for @harikesh-kolekar to understand how we're doing deprecations and talk about foodcritic rules.

btm requested review from thommay and tas50

December 13, 2017 14:05

harikesh-kolekar changed the title ~~[MSYS-726][In-Progress] Allow setting environment variables at the user level~~ [MSYS-726] Allow setting environment variables at the user level

NimishaS reviewed

View reviewed changes

spec/functional/resource/env_spec.rb Outdated

    
                      test_resource.value(env_value1)

                      test_resource.run_action(:create)

                      env_obj

                      expect(@env_obj.username.upcase).to eq(default_env_user)

NimishaS Dec 21, 2017

Can we assign the return value of env_obj to some variable instead of using the instance variable @env_obj?

NimishaS reviewed

View reviewed changes

spec/functional/resource/env_spec.rb Outdated

    
                        return @env_obj if @env_obj.username.split('\\').last.casecmp(test_resource.user) == 0

                      end

                    end

                    @env_obj = nil

NimishaS Dec 21, 2017 •

edited

Please return a value from this, instead of setting the instance variable. Probably use a method if required.

NimishaS suggested changes

View reviewed changes

spec/functional/resource/env_spec.rb Outdated

    
            @@ -55,6 +79,25 @@
          
                      expect(ENV[chef_env_test_lower_case]).to eq(env_value1)

                    end

                    it "should create an environment variable with default user System for action create" do

NimishaS Dec 21, 2017

It would be good to have a description "<#create_method_name>" block and club all the test cases related to create method in it.
We can avoid appending for action create in each spec.

spec/functional/resource/env_spec.rb Outdated

    
            @@ -65,6 +108,20 @@
          
                      expect(ENV[chef_env_test_lower_case]).to eq(env_value2)

                    end

                    it "should not modify an existing variable's value to a new value if the user are diff" do

NimishaS Dec 21, 2017

"should not modify an existing variable's value to a new value if the users are different"

spec/functional/resource/env_spec.rb Outdated

    
                      test_resource.key_name(chef_env_test_lower_case)

                      test_resource.user(default_env_user)

                      env_obj

                      expect(@env_obj.variablevalue).to eq(env_value1)

NimishaS Dec 21, 2017

We can probably use a before block here for creating the existing variable.

spec/functional/resource/env_spec.rb Outdated

    
                      expect(@env_obj).to eq(nil)

                    end

                    it "should not delete an user environment variable if user are not passed" do

NimishaS Dec 21, 2017

should not delete an user environment variable if user is not passed.

spec/functional/resource/env_spec.rb Outdated

    
                      test_resource.run_action(:delete)

                      test_resource.user(env_user)

                      env_obj

                      expect(@env_obj).not_to be_nil

NimishaS Dec 21, 2017

Please use before block or let for creating the existing environment variable.

spec/unit/provider/env_spec.rb Outdated

    
                  it "should set the key_name to the key name of the new resource" do

                    @provider.load_current_resource

                    expect(@provider.current_resource.key_name).to eq("FOO")

                  end

                  it "should check if the key_name exists" do

                  it "should check if the key_name and user exists" do

NimishaS Dec 21, 2017

"should check if the key_name and user exist"

spec/unit/provider/env_spec.rb Outdated

    
            @@ -152,14 +158,20 @@
          
                    @provider.action_modify

                  end

                  it "should call modify_group if the key exists and user are not equal" do

NimishaS Dec 21, 2017

"and users are not equal"

harikesh-kolekar force-pushed the harry/allow_setting_environment_variables_at_the_user branch 2 times, most recently from debdb22 to dbfa838 Compare

December 22, 2017 13:18

harikesh-kolekar force-pushed the harry/allow_setting_environment_variables_at_the_user branch from dbfa838 to 10577c9 Compare

January 8, 2018 12:10

tas50 reviewed

View reviewed changes

lib/chef/resources.rb Outdated

    
            @@ -35,7 +35,7 @@
          
              require "chef/resource/dnf_package"

              require "chef/resource/dsc_script"

              require "chef/resource/dsc_resource"

              require "chef/resource/env"

              require "chef/resource/windows_env"

Contributor

tas50 Jan 22, 2018

this should go in the right place alphabetically.

tas50 reviewed

View reviewed changes

lib/chef/resource/windows_env.rb Outdated

    
                    provides :env, os: "windows"

                  class WindowsEnv < Chef::Resource

                    resource_name :windows_env

                    provides :windows_env, os: "windows"

Contributor

tas50 Jan 22, 2018

We want to make sure to also include provides :env here so that any windows users with previously cookbooks will continue to work.

Contributor

lamont-granquist commented Jan 22, 2018

after some poking around i am satisfied that we never had a unix env resource before. on unix previously it blew up with NoSuchResourceType because the resource was windows-only, then the provider that was wired up on unix was a stub that raised notimplementederror. so busted all around, nobody could have ever used it.

want the back compat though so people on windows that were using 'env' can continue using 'env'.

Contributor

thommay commented Jan 22, 2018

Right, so this is going in the right direction, and clearly on non-Windows this has raised out of the box so there's no concern about deprecation there.
What we should do:

the resource should add a provides :env os: "windows".
There's no real need for the superclass any more, so just merge providers/env/windows.rb into providers/env.rb and then rename that to providers/windows_env.rb.
Delete the providers/env/ directory.

wrightp reviewed

View reviewed changes

spec/functional/resource/windows_env_spec.rb

    
            @@ -0,0 +1,285 @@
          
              #

wrightp Jan 25, 2018

I recommend doing a git mv here and then make the appropriate changes.

Author

harikesh-kolekar Jan 30, 2018

I have tried by git mv but still, in git file changes it shows file has been deleted you can see commit at 85ad3ee#diff-5903ffd7bafff40eae05e9c21380c6a0

harikesh-kolekar force-pushed the harry/allow_setting_environment_variables_at_the_user branch from caf9c6e to 4401519 Compare

January 29, 2018 12:11

harikesh-kolekar force-pushed the harry/allow_setting_environment_variables_at_the_user branch from 4401519 to 0b10937 Compare

January 29, 2018 12:56

thommay reviewed

View reviewed changes

lib/chef/provider/windows_env.rb Outdated

    
                    attr_accessor :key_exists

                    provides :env, os: "!windows"

                    provides :windows_env, os: "windows"

Contributor

thommay Jan 29, 2018

can you add

provides :env, os: "windows"

here too please?

thommay suggested changes

View reviewed changes

Contributor

thommay left a comment

We're pretty close here thank you!

lib/chef/providers.rb Outdated

@@ @@ -28,7 +28,7 @@ @@
               require "chef/provider/directory"
               require "chef/provider/dsc_script"
               require "chef/provider/dsc_resource"
-              require "chef/provider/env"
+              require "chef/provider/windows_env"

Contributor

thommay Jan 29, 2018

this file should be sorted alphabetically.

harikesh-kolekar added 6 commits

January 30, 2018 07:31


          Add user attribute to create env variable

c75f663

Signed-off-by: harikesh-kolekar <harikesh.kolekar@msystechnologies.com>


          update spec

13b2ea5

Signed-off-by: harikesh-kolekar <harikesh.kolekar@msystechnologies.com>


          update functional specs

e88d467

Signed-off-by: harikesh-kolekar <harikesh.kolekar@msystechnologies.com>


          delete env variable if present in current process

5809c04

Signed-off-by: harikesh-kolekar <harikesh.kolekar@msystechnologies.com>


          update specs as per nimisha comments

4b8580c

Signed-off-by: harikesh-kolekar <harikesh.kolekar@msystechnologies.com>


          rename env resource to windows_env

85ad3ee

Signed-off-by: harikesh-kolekar <harikesh.kolekar@msystechnologies.com>

harikesh-kolekar force-pushed the harry/allow_setting_environment_variables_at_the_user branch 5 times, most recently from ba8e126 to fcbc5a2 Compare

January 31, 2018 10:31


          require file as per alphabetical order

ed4b10b

Signed-off-by: harikesh-kolekar <harikesh.kolekar@msystechnologies.com>

harikesh-kolekar force-pushed the harry/allow_setting_environment_variables_at_the_user branch from fcbc5a2 to ed4b10b Compare

January 31, 2018 11:26

lamont-granquist approved these changes

View reviewed changes

Contributor

lamont-granquist commented Feb 12, 2018

@thommay merge if you think its good

lamont-granquist added the Status: Ready for Merge label

thommay approved these changes

View reviewed changes

thommay merged commit 2d38372 into chef:master

lock bot commented Apr 14, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

lock bot locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

tas50 tas50 left review comments

wrightp wrightp left review comments

NimishaS NimishaS requested changes

btm btm approved these changes

thommay thommay approved these changes

lamont-granquist lamont-granquist approved these changes