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

AWSParameterStore - Support Global Parameters #302

Merged
merged 16 commits into from
Sep 24, 2020

Conversation

codezninja
Copy link
Contributor

  • add support for global parameters

@codezninja
Copy link
Contributor Author

codezninja commented Sep 16, 2020

TODO

  • unit tests
  • docs
  • changelog

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2020

Codecov Report

Merging #302 into master will increase coverage by 0.54%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #302      +/-   ##
==========================================
+ Coverage   78.43%   78.98%   +0.54%     
==========================================
  Files          41       41              
  Lines        1048     1061      +13     
  Branches      244      247       +3     
==========================================
+ Hits          822      838      +16     
+ Misses        105       99       -6     
- Partials      121      124       +3     
Impacted Files Coverage Δ
src/ParameterStoreBuildWrapperPlugin.groovy 71.42% <80.00%> (+30.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90843f2...84d5cfc. Read the comment docs.

@codezninja codezninja marked this pull request as draft September 21, 2020 19:20
def result = ParameterStoreBuildWrapperPlugin.withGlobalParameter('/path/', [recursive: true, basename: 'relative'])

assertEquals([[path: '/path/', recursive: true, basename: 'relative']], result.globalParameterOptions)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this test. It shows that you can call addGlobalParameters with or without options.

@kmanning
Copy link
Collaborator

kmanning commented Sep 22, 2020

Code coverage is going down because the apply methods were not previously tested, and your change requires that more lines of code be added to apply (which therefore increases the number of untested code lines).

Let's tackle that by first creating contexts for apply in our test suites. Since we have 2 apply's - one for TerraformValidateStage and another for TerraformEnvironmentStage - let's create an "ApplyForTerraformValidateStage" context, and a "ApplyForTerraformEnvironmentStage" context, so that each method can be unit tested separately. You can see this sort of pattern here - https://github.com/manheim/terraform-pipeline/blob/master/test/TagPluginTest.groovy#L114

Looking at the behavior of apply(TerraformValidateStage stage), I think I might say that there are 2 behaviors.

  1. It does nothing if there are no global parameters
  2. It adds each parameter as a decoration to the validate stage

To sketch out what that might look like in code:

    public class ApplyForValidateStage {
        @Test
        public void addsNoDecorationWhenThereAreNoGlobalParameters() {
            .... 
        }

        @Test
        public void addsADecorationForEachParameter() {
            .... 
        }

Zooming in on what addsNoDecorationWhenThereAreNoGlobalParameters might test, I think I'd setup a

  • Given there are no global parameters
  • When I call apply(validatestage)
  • Then I should see that validatestage never had the decorate method called.

Zooming in on what addsADecorationForEachParameter might test, I think I'd setup a

  • Given I have multiple global parameters (let's say 3)
  • When I call apply(validatestage)
  • Then I should see that validate stage got decorated with each parameter (3 times, with each of the parameters I added).

^-- taking the above, I could follow a similar process for "ApplyForTerraformEnvironmentStage". Looking at the implementation of that method, there's slightly more behavior. There's everything that's done to ValidateStage, and then some.

  1. It does nothing if there are no global parameters
  2. It adds each global parameter as a decoration to the environment stage during the PLAN command
  3. It adds each global parameter as a decoration to the environment stage during the APPLY command
  4. It adds each environment-specific parameter as a decoration to the environment stage during the PLAN command
  5. It adds each environment-specific parameter as a decoration to the environment stage during the APPLY command

Each of those 5 behaviors could potentially be tested through a distinct unit test. If you wanted to limit the scope of tests to just what you changed, you could focus on tests 1, 2, 3, and say that 4 and 5 should have already been tested prior to your change.

@codezninja
Copy link
Contributor Author

codezninja commented Sep 22, 2020

I need a second 👀 at the tests for 2-5

2-3 should be tested here
4-5 should be tested here

@codezninja codezninja marked this pull request as ready for review September 23, 2020 02:33
@codezninja codezninja requested a review from kmanning September 23, 2020 02:33
@codezninja codezninja added this to the v5.12 milestone Sep 23, 2020
@codezninja codezninja requested a review from kmanning September 23, 2020 23:40
@kmanning
Copy link
Collaborator

Awesome! Thanks for the contribution, this feels like a super useful feature! :shipit:

@kmanning kmanning linked an issue Sep 24, 2020 that may be closed by this pull request
@codezninja codezninja merged commit f80fbe1 into manheim:master Sep 24, 2020
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.

Support Global ParameterStore path for all stages
3 participants