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

Let raw templates use previous Parameter values #615

Merged
merged 17 commits into from
Jul 8, 2018

Conversation

phobologic
Copy link
Member

@phobologic phobologic commented Jul 1, 2018

This changes a couple of things:

  1. We now use the UsePreviousValue argument for parameters that exist in an existing stack, rather than getting the value there and providing it ourselves. This is more "cloudformation-y".
  2. The raw blueprint no longer handles missing variables on it's own - it didn't need to, because stacker.actions.build._handle_missing_parameters already handles this for us.

Fixes #593

Before this we actually provided the value from the existing parameter,
but that's not really what cloudformation wants, and it can lead to
issues when using raw templates.

Fixes #593
We check for this later in `stacker/action/build.py` with
`_handle_missing_parameters`.  This allows raw stacks to use the
`UsePreviousValue` option for parameters if a previous stack exists, and
it has the parameter, otherwise it blows up.
@phobologic
Copy link
Member Author

BTW, if anyone has a better idea than using a class to represent when we should UsePreviousValue I'm all ears. This was the cleanest way I could think to make this change without having to change existing tests.

@phobologic phobologic requested a review from ejholmes July 1, 2018 20:13
@phobologic
Copy link
Member Author

@troyready would love your eyes on this as well, as it changes the way the raw blueprints work.

@phobologic phobologic added this to the 1.4 milestone Jul 1, 2018
@troyready
Copy link
Contributor

I really, really like the simplicity of it.

@phobologic
Copy link
Member Author

phobologic commented Jul 1, 2018

This has a small potential bug in it, though it's more a question of what people would expect the behavior to be. As it's written now, if a stack has a Default parameter and you do not pass anything in for it, then on update, it will use that Default - rather than UsePreviousValue. This means that if the template ever changes it's Default, that is what will be used, causing an update.

I think the right order of resolution for Parameters should be:

  • If a parameter is passed in via the config, use that.
  • If there is an existing stack and it has the Parameter already, UsePreviousValue
  • If there is a Default on the Parameter, use that value.

The way it is currently is:

  • If a parameter is passed in via the config, use that.
  • If there is a Default on the Parameter, use that value.
  • If there is an existing stack and it has the Parameter already, UsePreviousValue

Which sounds right to everyone? @troyready @ejholmes @aarongorka ?

Copy link
Contributor

@ejholmes ejholmes left a comment

Choose a reason for hiding this comment

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

Change looks good, but can we add a functional test that would break before this change?

New order of operations:

- If a parameter is passed in via the config, use that.
- If there is an existing stack and it has the Parameter already, UsePreviousValue
- If there is a Default on the Parameter, use that value.
@phobologic
Copy link
Member Author

phobologic commented Jul 7, 2018

Ok, I have a test suite now that fails in master, but is fixed here.

in master:

# TESTS=28 make -C tests test
./stacker.yaml.sh | stacker build -
[2018-07-07T13:14:15] Using default AWS provider mode
[2018-07-07T13:14:17] stackerFunctionalTests: skipped (nochange)
bats  test_suite/28_*
 ✗ stacker build - raw template parameter resolution
   (from function `assert_has_line' in file test_suite/../test_helper.bash, line 39,
    in test file test_suite/28_stacker_build-raw_template_parameter_resolution.bats, line 68)
     `assert_has_line "vpc: skipped (nochange)"' failed
   PWD: /Users/mike/git/cloudtools/stacker/tests
   $ stacker build /dev/fd/63
   [2018-07-07T13:14:20] Using default AWS provider mode
   [2018-07-07T13:14:22] vpc: submitted (creating new stack)
   [2018-07-07T13:14:27] vpc: complete (creating new stack)

   $ stacker build /dev/fd/63
   [2018-07-07T13:14:28] Using default AWS provider mode
   [2018-07-07T13:14:29] vpc: submitted (updating existing stack)
   [2018-07-07T13:14:33] vpc: complete (updating existing stack)

   $ stacker destroy --force /dev/fd/63
   [2018-07-07T13:14:34] Using default AWS provider mode
   [2018-07-07T13:14:35] vpc: submitted (submitted for destruction)
   [2018-07-07T13:14:38] vpc: complete (stack destroyed)


1 test, 1 failure
make: *** [test] Error 1

in this branch:

# TESTS=28 make -C tests test
./stacker.yaml.sh | stacker build -
[2018-07-07T13:13:21] Using default AWS provider mode
[2018-07-07T13:13:23] stackerFunctionalTests: skipped (nochange)
bats  test_suite/28_*
 ✓ stacker build - raw template parameter resolution

1 test, 0 failures

This makes this PR rely on #597 since I got sick of having to run all the tests :) You can ignore all the functional tests except for 28 (https://github.com/cloudtools/stacker/pull/615/files#diff-2a1538382726a9ab337f7c118a526539) - once #597 is merged, this should clean up.

@phobologic phobologic merged commit c1ccb5e into master Jul 8, 2018
@phobologic phobologic deleted the really_use_existing_parameters branch July 8, 2018 00:20
phrohdoh pushed a commit to phrohdoh/stacker that referenced this pull request Dec 18, 2018
* Add helper Makefile for more easier integration tests

* no need for cd, -C is better

* Use UsePreviousValue on existing parameters

Before this we actually provided the value from the existing parameter,
but that's not really what cloudformation wants, and it can lead to
issues when using raw templates.

Fixes cloudtools#593

* Don't catch missing variables in the raw blueprint

We check for this later in `stacker/action/build.py` with
`_handle_missing_parameters`.  This allows raw stacks to use the
`UsePreviousValue` option for parameters if a previous stack exists, and
it has the parameter, otherwise it blows up.

* Don't manually grab default, let cloudformation handle that

* Fixes the order of operations for missing params

New order of operations:

- If a parameter is passed in via the config, use that.
- If there is an existing stack and it has the Parameter already, UsePreviousValue
- If there is a Default on the Parameter, use that value.

* Make it possible to run a single test case

* Add specific test examples

* No longer have _all_ method

* Functional test for this fix

* Comment in yaml template on what it is used for

* Fix functional tests for new functional test method
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