aws-ia / taskcat

Test all the CloudFormation things! (with TaskCat)
https://aws-ia.github.io/taskcat/
Apache License 2.0
1.17k stars 213 forks source link

PROPOSAL - per-test/per-region parameters #494

Open jaymccon opened 4 years ago

jaymccon commented 4 years ago

At present parameters defined in the global config, used for overriding parameters defined in tests/projects, is a simple key-value representation of parameter names and values. where ever the name matches a parameter, it is overridden. This does not allow for providing per-test or per-region overrides. An example of this would be where a Quick Start requires a region specific ID as a parameter value.

The proposed change has two components:

  1. Allow nested parameters in parameters under project and tests. If the value of a key is a dict, and the key is a region name, then the parameters defined under it are only applied to that region. For example:
tests:
  some-test:
    parameters:
      eu-west-2:
        myParam: some-value-for-this-region
      myParam: default-value

This could alternatively be done by allowing sub-keys on the region lists instead of the parameters.

  1. Create a new pseudo parameter that points to an override key. For example:
tests:
  some-test:
    parameters:
      myParam: $[override_myVal]

in the global config we would have myVal defined, and the value for that would be substituted.

general:
  parameters:
    myVal: this-value-will-go-to-myParam

with these two changes we would be able to do complex overrides, of potentially secret values:

project:
  parameters:
    myParam: $[override_defaultVal]
tests:
  some-test:
    parameters:
      eu-west-2:
        myParam: $[override_regionVal]
      myParam: $[override_testVal]

Thoughts ?

jaymccon commented 4 years ago

thinking about this a bit more, I prefer making 1. use the regions list. Provides slightly more flexibility, and means we don't need to deal with complicating the validation on parameter keys being either a literal key or a region name.

andrew-glenn commented 4 years ago

Is this a blocker, or nice-to-have?

That aside, this appears as a breaking change at first glance, since we assume key:value under parameters... Will we introduce backwards compatibility ?

Assuming we will introduce backwards compat, this kinda-sorta goes against the grain of taskcat. In my mind, taskcat is a tool that tests the same configuration (test definition) across multiple regions. I'm all for flexibility, but I'm concerned about corner cases, especially in CICD pipelines.

jaymccon commented 4 years ago

It's a blocker to be able to run tests in ci for connect integrations (they require a parameter with an id of an already existing connect instance in the same region). It's a nice to have for me because I have other use cases where it will simplify my life.

It's not a breaking change.

andrew-glenn commented 4 years ago

It's not a breaking change.

Without additional logic, it is. Current test definitions expect..

parameters:
  key: value

Not

parameters:
  region_name:
    key:value

We'll need to add something like..

if isinstance(value, dict):
   (...)
elif isinstance(value, str):
   (...)
jaymccon commented 4 years ago

since we assume key:value under parameters

parameters can be any type except dict at present (cfn doesn't accept dict). All this will introduce is that IF a parameter is of type dict, it is treated specially. At present taskcat will just crash if you give it a dict.

andrew-glenn commented 4 years ago

since we assume key:value under parameters

parameters can be any type except dict at present (cfn doesn't accept dict). All this will introduce is that IF a parameter is of type dict, it is treated specially. At present taskcat will just crash if you give it a dict.

More ☕️ needed. Happy to review a PR when it's ready.

jaymccon commented 4 years ago

I'm thinking it would be best to put the regional params under regions instead. This way we can do per region overrides for all tests (put it in project) or for a specific test (put it in test).

Example:

project:
  regions:
    - us-east-1:
        myParam: ue1val
    - ca-central-1
tests:
  basic-test:
    parameters:
      myParam: basic-test-default
  override-test:
    regions:
      - us-west-2:
          myParam: override-test-uw2val
      - us-east-1:
          myParam: override-test-ue1val
      - ca-central-1
  parameters:
    myParam: override-test-default

would result in the following stacks/parameters:

basic-test - us-east-1 - myParam: ue1val basic-test - ca-central-1 - myParam: basic-test-default override-test - us-west-2 - myParam: override-test-uw2val override-test - us-east-1 - myParam: override-test-ue1val override-test - ca-central-1 - myParam: basic-test-default

@avattathil @andrew-glenn thoughts ?

andrew-glenn commented 4 years ago

@jaymccon

I'm good with this. It just means a few more calls to the template_params stuff. Would it make sense to craft a list of parameter sources, like what's done in the Config object? Merge the dictionaries for each region, then pass it along for conversion? I want to make sure we nail down inheritance without any complications.

Global Overrides -> Project Overrides -> RegionParams -> TestParams -> ProjectParams -> Final dictionary -> psuedo-param conversion -> launch stack?

andrew-glenn commented 4 years ago

Just to circle back - I'm running into a situation where this would mitigate pain. Any questions on the value-add I had are now gone.

zetas commented 4 years ago

This would be awesome for us as well. We ship out CF stacks to customers to deploy our software so we allow them to choose the VPC, Subnets, etc so for testing in different regions we would need these to be per-region.