#85: Feature/use environment without preserve #86

homelinen · 2017-02-23T11:28:55Z

Overview

This pull request changes the behaviour of the --environment flag when used on its own.

Solve the problem described in #85. Allows the --environment flag to be used without the --preserve-environments flag.

Tries to maintain the current functionality by ignoring this parameter when used with --create-symlinks.

Doesn't modify the existing behaviour of --preserve-environments

Checklist

Make sure that all of the tests pass, and fix any that don't. Just run rake in your checkout directory, or review the CI job triggered whenever you push to a pull request.
Make sure that there is 100% test coverage by running rake coverage:spec or ignoring untestable sections of code with # :nocov comments. If you need help getting to 100% coverage please ask; however, don't just submit code with no tests.
If you have added a new command line option, we would greatly appreciate a corresponding integration test that exercises it from start to finish. This is optional but recommended.
If you have added any new gem dependencies, make sure those gems are licensed under the MIT or Apache 2.0 license. We cannot add any dependencies on gems licensed under GPL.
If you have added any new gem dependencies, make sure you've checked in a copy of the .gem file into the vendor/cache directory.

homelinen · 2017-02-23T11:30:39Z

Will try to write an integration test and test against my existing repository. (The puppet version was causing some troubles, I'll try and figure out how to make that work).

Integration tests don't work locally because I presume github.net is an internal domain so wanted to test again Travis.

homelinen · 2017-02-23T11:32:02Z

lib/octocatalog-diff/catalog-util/builddir.rb

@@ -330,7 +334,7 @@ def install_ssl_client_password(logger, password)
      end

      def environment
-        @options[:preserve_environments] ? @options.fetch(:environment, 'production') : 'production'
+        @options.fetch(:environment, 'production')


Maybe there should be a protection here to block environment and create-symlinks being used together, as the debug output on 89 states.

I looked into this and you're actually very close here. There is a method in https://github.com/github/octocatalog-diff/blob/master/lib/octocatalog-diff/catalog/computed.rb#L82-L84 that needs to be updated to match this change. Also some spec tests need to be updated for the new behavior.

Have a look at https://gist.github.com/kpaulisse/7bda51e395aab49585aa9933d78fe724 for what I had to do to get the tests passing. Hopefully this is a push in the right direction - I'm 👍 to the change in general.

homelinen · 2017-02-23T11:32:42Z

spec/octocatalog-diff/tests/catalog-util/builddir_spec.rb

@@ -142,10 +145,11 @@
        end

        context 'without logger' do
-          it 'should install default symlink' do
+          it 'should install directory symlink' do


This should still say default. Oops.

homelinen · 2017-03-06T17:13:05Z

lib/octocatalog-diff/catalog/computed.rb

@@ -80,7 +80,7 @@ def compilation_dir

      # Environment used to compile catalog
      def environment
-        @opts[:preserve_environments] ? @opts.fetch(:environment, 'production') : 'production'
+        @options.fetch(:environment, 'production')


Oops, opts.

homelinen · 2017-03-07T17:00:17Z

Will look at those failed tests over the next few days. Haven't checked them yet.

But managed to get environment working fully on my own repo so that's good progress.

kpaulisse · 2017-03-08T03:46:56Z

Hey @homelinen I looked at the test results and this one looks like it might just be a broken test:

  1) preserve environments integration without --preserve-environments set with --environment set should exit without error
     Failure/Error: expect(@result.exitcode).to eq(2), OctocatalogDiff::Integration.format_exception(@result)
     
       expected: 2
            got: 0
     
       (compared using ==)
     # ./spec/octocatalog-diff/integration/preserve_environments_spec.rb:50:in `block (4 levels) in <top (required)>'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.5.0/lib/rspec/retry.rb:112:in `block in run'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.5.0/lib/rspec/retry.rb:101:in `loop'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.5.0/lib/rspec/retry.rb:101:in `run'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.5.0/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.5.0/lib/rspec/retry.rb:30:in `block (2 levels) in setup'

Basically 2 means "successful, with diffs" and 0 means "successful, no diffs". It looks like you actually changed the original test from 0 to 2 (and I vaguely remember this being in my patch when I first tried your code on my machine but I didn't look into it too deeply at the time). I just re-ran that on my machine and got the same failure as travis is seeing, so if you revert just that one change the test will hopefully pass as it did before.

kpaulisse

I'm glad to hear this is mostly working for you. I pointed out the test that needs fixing, and made one suggestion about the logger.

Once you have verified code that works for you and let me know you're done, I'll run the updated code within GitHub for a day or so to make sure everything looks good (we end up running this up to 10k times per day in various CI) and then merge. Thanks!

kpaulisse · 2017-03-08T03:48:12Z

spec/octocatalog-diff/integration/preserve_environments_spec.rb

            '--hiera-path-strip', '/var/lib/puppet', '--no-parallel'
          ]
        )
      end

      it 'should exit without error' do
-        expect(@result.exitcode).to eq(0), OctocatalogDiff::Integration.format_exception(@result)
+        expect(@result.exitcode).to eq(2), OctocatalogDiff::Integration.format_exception(@result)


This one should be reverted as per my earlier comment - change the 2 back to a 0 here.

kpaulisse · 2017-03-08T03:49:47Z

spec/octocatalog-diff/tests/catalog-util/builddir_spec.rb

+      context 'with --environment' do
+        context 'with logger' do
+          it 'should install a symlink to the given environment' do
+            logger = double('Logger')


The pattern elsewhere in the tests is to actually construct a real logger using:

logger, logger_str = OctocatalogDiff::Spec.setup_logger

You can then also test logger_str.string to be matching text if you want to verify log messages.

I realize that line 127 didn't use this pattern - must have been one I missed when I went through and tried to update all of them long, long ago... Would you mind doing it the new way? 😸

kpaulisse · 2017-03-12T16:41:59Z

@homelinen the code looks fine now. Are you done with the development here? If so I'll test this internally within GitHub for a couple days, and then merge it. Please advise.

homelinen · 2017-03-13T10:46:23Z

@kpaulisse Yeah, I'm done with the development, I'm happy for internal testing to go ahead.

I realise one thing missing is a doc change to the Advanced Environments page. Would you want that in this PR or separate?

Happy to work it out either, sorry I forgot about it before.

kpaulisse · 2017-03-13T15:31:28Z

Feel free to do a separate PR for doc updates. I don't bump version numbers for that.

We are testing this version within GitHub today and if all goes well I'll merge it tomorrow. Thanks again!

…erve Bump version and changelog for #86

Calum Gilchrist added 5 commits February 22, 2017 16:47

Remove the Switch and rely on Environment Option

87d19bd

Create an Environment when environment switch is given

948db9f

Add a Test Case for the New Environment Behaviour

5b3f1ee

Add Logging around using create-symlinks without environment

6c2cfd2

Actually Return the value of install_dir_symlink

3f2093c

homelinen commented Feb 23, 2017

View reviewed changes

Calum Gilchrist added 4 commits March 6, 2017 16:48

Fix a Bad Test Description

05115a9

Compile the Correct Environment

13b3417

Use Environment specific Hiera in Tests

1237ef8

Update Catalog Compute Spec for Environments Behaviour

9d70936

homelinen commented Mar 6, 2017

View reviewed changes

Calum Gilchrist added 2 commits March 6, 2017 17:13

Use a Variable that Actually Exists

bc0f906

Allow Passing Environment into the Puppet Command

afe2aa3

kpaulisse suggested changes Mar 8, 2017

View reviewed changes

Calum Gilchrist added 2 commits March 10, 2017 17:10

Revert Unneeded Test Change

df5353a

Use the New Style of Logger Spec in Tests

ba0e21c

homelinen force-pushed the feature/use_environment_without_preserve branch from 20bfdeb to ba0e21c Compare March 10, 2017 17:31

kpaulisse mentioned this pull request Mar 12, 2017

Bump version and changelog for #86 #92

Merged

kpaulisse merged commit ba0e21c into github:master Mar 15, 2017

kpaulisse added a commit that referenced this pull request Mar 15, 2017

Merge pull request #92 from github/kpaulisse-environment-without-pres…

b024941

…erve Bump version and changelog for #86

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#85: Feature/use environment without preserve #86

#85: Feature/use environment without preserve #86

homelinen commented Feb 23, 2017 •

edited

Loading

homelinen commented Feb 23, 2017

homelinen Feb 23, 2017

kpaulisse Feb 26, 2017 •

edited

Loading

homelinen Feb 23, 2017

homelinen Mar 6, 2017

homelinen commented Mar 7, 2017

kpaulisse commented Mar 8, 2017

kpaulisse left a comment

kpaulisse Mar 8, 2017

kpaulisse Mar 8, 2017

kpaulisse commented Mar 12, 2017

homelinen commented Mar 13, 2017

kpaulisse commented Mar 13, 2017

#85: Feature/use environment without preserve #86

#85: Feature/use environment without preserve #86

Conversation

homelinen commented Feb 23, 2017 • edited Loading

Overview

Checklist

homelinen commented Feb 23, 2017

homelinen Feb 23, 2017

Choose a reason for hiding this comment

kpaulisse Feb 26, 2017 • edited Loading

Choose a reason for hiding this comment

homelinen Feb 23, 2017

Choose a reason for hiding this comment

homelinen Mar 6, 2017

Choose a reason for hiding this comment

homelinen commented Mar 7, 2017

kpaulisse commented Mar 8, 2017

kpaulisse left a comment

Choose a reason for hiding this comment

kpaulisse Mar 8, 2017

Choose a reason for hiding this comment

kpaulisse Mar 8, 2017

Choose a reason for hiding this comment

kpaulisse commented Mar 12, 2017

homelinen commented Mar 13, 2017

kpaulisse commented Mar 13, 2017

homelinen commented Feb 23, 2017 •

edited

Loading

kpaulisse Feb 26, 2017 •

edited

Loading