(CODEMGMT-1415) Sundry Improvements #1211

justinstoller · 2021-08-30T22:26:02Z

Fixes regression where on some module content would be deleted on subsequent runs when not using --modules
Renames --assume-unchanged flag to --incremental
Adds CHANGELOG entry

Still to do

Acceptance test

Magisus

I think the first commit could use a short description of the bug.

Are you planning to add the acceptance test as part of this PR, or should we merge this first to unblock code-manager's unit tests?

justinstoller · 2021-08-30T23:48:55Z

I was planning on doing the acceptance test as part of this work. We should see how long it takes me to run the damned things, if I don't have it by tomorrow we can merge, I guess.

Magisus · 2021-08-31T00:03:30Z

I'm fine waiting for now, I don't think we're landing anything more in code-manager right away.

mwaggett

+1 to updating the commit message to describe the bug

spec/unit/action/deploy/environment_spec.rb

In commit 9d0579f Environments stopped interrogating their Puppetfile for managed content needed to calculate their purge exclusions, but moved to using the data returned by the ModuleLoader when loading content. However, if content was not loaded prior to attempting to purge the environment then the environment would purge much of the Puppetfile managed content. This would happen when running `r10k deploy environment` (without `--modules`) on an existing environment. To resolve this we now conditionally load module data if it hasn't been loaded yet when resolving our purge exclusions.

Concerns were raised that `--assume-unchanged` required knowing too much about how R10K internals work and that users would be well aware of this kind of behavior from back up tools where the `--incremental` is the standard terminology.

When evaluating how to test `--incremental` behavior it was decided that the existing acceptance tests lacked basic sanity tests (there was an uncaught bug when running a simple `r10k deploy environment` twice). This provides basic acceptance level testing for running `r10k deploy environment` with and without `--modules` and `--incremental`.

Co-authored-by: Molly Waggett <[email protected]>

CLAassistant · 2021-09-01T17:03:59Z

All committers have signed the CLA.

When implementing and refactoring the Puppetfile ModuleLoader logging that happened on instantiation of the Puppetfile was moved to the instantiation of the ModuleLoader. However, the a ModuleLoader is instantiated for both the legacy Puppetfile as well as the owning Environment directly, causing the log lines to be emitted twice.

justinstoller · 2021-09-01T17:27:35Z

I'm having a lot of trouble running the acceptance tests locally. I think it's at a place where it can be reviewed at least while I continue to try and run the test.

I also believe I've address the comments so far and added a new commit cleaning up some logging.

Magisus · 2021-09-01T18:09:35Z

Cool, I'll take a look, and also try to run your test.

Magisus

I think this looks good so long as the test passes. It checks all the boxes I would want to see for the basic workflow.

mwaggett · 2021-09-02T00:13:19Z

integration/tests/basic_functionality/basic_deployment.rb

+end
+
+step 'Test --modules --incremental updates changed modules' do
+  on(master, "#{r10k_fqp} deploy environment production --modules --incremental --verbose=debug1") do |result|


Without a new Puppetfile, this deploy isn't actually changing anything, is it? Is there a better way we can test this behavior other than checking the log messages? Just trying to think in terms of actual workflows, since I don't imagine someone would run a deploy with --modules and then again with --modules --incremental without at least making some sort of change in between?

Yeah, I struggled with this. The end result of running --modules vs --modules --incremental should be the same, modules that should be updated will be updated. However, the how it happens is different, with some modules being skipped completely and some still being checked. So the logging shows us at least that the static version checking is correct (for a forge module) and that unchanged static modules are skipped.

If you think I should I could include all the permutations of different types of versions as well as those that have changed definitions? Or are you mainly concerned about those modules whose definition changed on disk part? I could include one of those in the Puppetfile.

I'll add a module that is updated in this invocation to test the changeable vs static, changed vs static, unchanged. Let me know if you'd like more permutations of static version checking (fwiw, there is some unit tests around that: 6f46384#diff-1bc365ddca4e6e4a7ba95ff4d655c036faa16ec1025e35bc559476265a5719aaR344-R389)

I think what you added looks good! I'm satisfied with the various permutations we test here.

integration/tests/basic_functionality/basic_deployment.rb

justinstoller · 2021-09-02T17:06:11Z

I've tested this test, with current HEAD of pe-r10k (80392485):

              - Test Case Summary for suite 'tests' -
       Total Suite Time: 10.60 seconds
      Average Test Time: 10.60 seconds
              Attempted: 1
                 Passed: 0
                 Failed: 1
                Errored: 0
                Skipped: 0
                Pending: 0
                  Total: 1

      - Specific Test Case Status -
        
Failed Tests Cases:
  Test Case tests/basic_functionality/basic_deployment.rb reported: #<Minitest::Assertion: Expected module content in Puppetfile was inappropriately purged>
    Test line: tests/basic_functionality/basic_deployment.rb:102:in `block (3 levels) in run_test'
Errored Tests Cases:
Skipped Tests Cases:
Pending Tests Cases:

As well as this content (e5b714d29):


              - Test Case Summary for suite 'tests' -
       Total Suite Time: 13.73 seconds
      Average Test Time: 13.73 seconds
              Attempted: 1
                 Passed: 1
                 Failed: 0
                Errored: 0
                Skipped: 0
                Pending: 0
                  Total: 1

      - Specific Test Case Status -
        
Failed Tests Cases:
Errored Tests Cases:
Skipped Tests Cases:
Pending Tests Cases:

justinstoller · 2021-09-02T17:07:17Z

Running the whole suite now

mwaggett

mwaggett · 2021-09-02T17:45:54Z

Feel free to merge once you're satisfied with your suite run.

justinstoller requested a review from a team August 30, 2021 22:26

Magisus approved these changes Aug 30, 2021

View reviewed changes

mwaggett reviewed Aug 31, 2021

View reviewed changes

spec/unit/action/deploy/environment_spec.rb Outdated Show resolved Hide resolved

justinstoller and others added 5 commits September 1, 2021 09:54

(CODEMGMT-1415) Update CHANGELOG

e8366b2

Update spec/unit/action/deploy/environment_spec.rb

4acfacb

Co-authored-by: Molly Waggett <[email protected]>

justinstoller force-pushed the CODEMGMT-1415-fixups branch from 1fc43c3 to 4acfacb Compare September 1, 2021 17:03

Magisus approved these changes Sep 1, 2021

View reviewed changes

mwaggett reviewed Sep 2, 2021

View reviewed changes

(CODEMGMT-1415) Improve acceptance test

5042804

justinstoller force-pushed the CODEMGMT-1415-fixups branch from 99535d1 to 5042804 Compare September 2, 2021 16:16

mwaggett approved these changes Sep 2, 2021

View reviewed changes

justinstoller merged commit 4a832b7 into puppetlabs:main Sep 2, 2021

justinstoller deleted the CODEMGMT-1415-fixups branch September 2, 2021 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(CODEMGMT-1415) Sundry Improvements #1211

(CODEMGMT-1415) Sundry Improvements #1211

justinstoller commented Aug 30, 2021 •

edited

Loading

Magisus left a comment

justinstoller commented Aug 30, 2021

Magisus commented Aug 31, 2021

mwaggett left a comment

CLAassistant commented Sep 1, 2021 •

edited

Loading

justinstoller commented Sep 1, 2021

Magisus commented Sep 1, 2021

Magisus left a comment

mwaggett Sep 2, 2021

justinstoller Sep 2, 2021

justinstoller Sep 2, 2021

mwaggett Sep 2, 2021

justinstoller commented Sep 2, 2021

justinstoller commented Sep 2, 2021

mwaggett left a comment

mwaggett commented Sep 2, 2021

(CODEMGMT-1415) Sundry Improvements #1211

(CODEMGMT-1415) Sundry Improvements #1211

Conversation

justinstoller commented Aug 30, 2021 • edited Loading

Magisus left a comment

Choose a reason for hiding this comment

justinstoller commented Aug 30, 2021

Magisus commented Aug 31, 2021

mwaggett left a comment

Choose a reason for hiding this comment

CLAassistant commented Sep 1, 2021 • edited Loading

justinstoller commented Sep 1, 2021

Magisus commented Sep 1, 2021

Magisus left a comment

Choose a reason for hiding this comment

mwaggett Sep 2, 2021

Choose a reason for hiding this comment

justinstoller Sep 2, 2021

Choose a reason for hiding this comment

justinstoller Sep 2, 2021

Choose a reason for hiding this comment

mwaggett Sep 2, 2021

Choose a reason for hiding this comment

justinstoller commented Sep 2, 2021

justinstoller commented Sep 2, 2021

mwaggett left a comment

Choose a reason for hiding this comment

mwaggett commented Sep 2, 2021

justinstoller commented Aug 30, 2021 •

edited

Loading

CLAassistant commented Sep 1, 2021 •

edited

Loading