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

Add stage action namespace to resource_aws_codepipeline #11910

Merged
merged 11 commits into from
Apr 22, 2020

Conversation

suchpuppet
Copy link
Contributor

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #11418

Release note for CHANGELOG:

Adds support for action stage namespace.  This allows variable namespacing for use in other stages.

Output from acceptance testing:

⚡ [~/development/terraform-providers/terraform-provider-aws]--> make testacc TESTARGS='-run=TestAccAWSCodePipelineWithNamespace'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAWSCodePipelineWithNamespace -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSCodePipelineWithNamespace
=== PAUSE TestAccAWSCodePipelineWithNamespace
=== CONT  TestAccAWSCodePipelineWithNamespace
--- PASS: TestAccAWSCodePipelineWithNamespace (46.47s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       47.927s
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/flatmap      0.506s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags 0.059s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/service/eks/token    0.091s [no tests to run]
?       github.com/terraform-providers/terraform-provider-aws/awsproviderlint   [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes    0.260s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes/AWSAT001   0.286s [no tests to run]
⚡ [~/development/terraform-providers/terraform-provider-aws]--> git pull'
...

The ability to use namespace in build actions is necessary to be able to do things like use the Github provider and reference the BranchName from the source stage. I ran into this when trying to use a Lambda to dynamically create pipelines similar to what is shown here: https://github.com/nicolai86/awesome-codepipeline-ci

When using the GitHub source provider, it wasn't possible to do that because I was not able to use namespaced variables from the source stage in the build stage without the updates in this pull request. When testing the updates in the pull request out, the pipeline was successfully created with the proper variable namespace and my build stage was able to use them.

@suchpuppet suchpuppet requested a review from a team February 5, 2020 19:16
@ghost ghost added size/L Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. service/codepipeline Issues and PRs that pertain to the codepipeline service. documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Feb 5, 2020
@jacobeatsspam
Copy link

@bflad Could this get some attention? It's been waiting a long time and seems to have a lot of support.

@bflad
Copy link
Contributor

bflad commented Apr 2, 2020

Hi @jacobisaliveandwell 👋 There are some inflight changes to the aws_codepipeline resource here: #12549

After that's been merged, we can take a look at this one. Thanks.

@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Apr 2, 2020
@ilyalukyanov
Copy link

@suchpuppet would you be able to resolve conflicts please? Now that #12549 has been merged this can proceed.

Thanks for making the change, btw. It's needed like air.

@nickaguilarh

This comment has been minimized.

@suchpuppet
Copy link
Contributor Author

@ilyalukyanov Sorry for the delay. Been busy. Conflicts have been resolved. Thanks for getting attention on this change as well.

t.Skip("Environment variable GITHUB_TOKEN is not set")
}

var p1, p2 codepipeline.PipelineDeclaration
Copy link

Choose a reason for hiding this comment

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

Lint is failing because p2 is declared but never used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'll remove that.

Comment on lines 399 to 400
resource.TestMatchResourceAttr(resourceName, "arn",
regexp.MustCompile(fmt.Sprintf("^arn:aws:codepipeline:[^:]+:[0-9]{12}:test-pipeline-%s", name))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you change this to use testAccMatchResourceAttrRegionalARN instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@@ -434,6 +442,10 @@ func flattenAwsCodePipelineStageActions(actions []*codepipeline.ActionDeclaratio
values["region"] = *action.Region
}

if action.Namespace != nil {
values["namespace"] = *action.Namespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you change this to values["namespace"] = aws.StringValue(action.Namespace)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Config: testAccAWSCodePipelineConfigWithNamespace(name),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSCodePipelineExists(resourceName, &p1),
resource.testAccMatchResourceAttrRegionalARN(resourceName, "arn", regexp.MustCompile(fmt.Sprintf("^arn:aws:codepipeline:[^:]+:[0-9]{12}:test-pipeline-%s", name))),
Copy link

@nl-brett-stime nl-brett-stime Apr 13, 2020

Choose a reason for hiding this comment

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

To fix the linting/tests in Travis, I believe you need to remove the resource. prefix and add a third argument with the value of "codepipeline" here:

testAccMatchResourceAttrRegionalARN(resourceName, "arn", "codepipeline", regexp.MustCompile(fmt.Sprintf("^arn:aws:codepipeline:[^:]+:[0-9]{12}:test-pipeline-%s", name))),

@nl-brett-stime
Copy link

@bflad It looks like this can move forward now that #12549 is merged. I'm not sure what the right etiquette is for moving someone else's PR (@suchpuppet) forward but I'm available to help however I can.

@nl-brett-stime
Copy link

FWIW, I've filed a PR for @suchpuppet 's fork: suchpuppet#1

...using my fix for the Travis lint/test stages: nl-brett-stime@b6dfbb9

Fixes testAccMatchResourceAttrRegionalARN
@bflad bflad self-assigned this Apr 16, 2020
@bflad bflad added this to the v2.59.0 milestone Apr 16, 2020
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Thanks so much for this, @suchpuppet 🚀

Output from acceptance testing:

--- PASS: TestResourceAWSCodePipelineExpandArtifactStoresValidation (0.00s)
--- PASS: TestAccAWSCodePipelineWithNamespace (28.70s)
--- PASS: TestAccAWSCodePipeline_deployWithServiceRole (32.42s)
--- PASS: TestAccAWSCodePipelineWebhook_basic (38.44s)
--- PASS: TestAccAWSCodePipelineWebhook_UpdateAuthenticationConfiguration_SecretToken (38.75s)
--- PASS: TestAccAWSCodePipeline_emptyArtifacts (41.79s)
--- PASS: TestAccAWSCodePipeline_multiregion_basic (45.82s)
--- PASS: TestAccAWSCodePipelineWebhook_unauthenticated (48.52s)
--- PASS: TestAccAWSCodePipelineWebhook_ipAuth (52.47s)
--- PASS: TestAccAWSCodePipelineWebhook_tags (52.76s)
--- PASS: TestAccAWSCodePipeline_basic (57.69s)
--- PASS: TestAccAWSCodePipeline_tags (60.91s)
--- PASS: TestAccAWSCodePipeline_multiregion_Update (66.88s)
--- PASS: TestAccAWSCodePipeline_multiregion_ConvertSingleRegion (69.45s)

@bflad bflad merged commit 4a9cc12 into hashicorp:master Apr 22, 2020
@ghost
Copy link

ghost commented Apr 24, 2020

This has been released in version 2.59.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented May 23, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators May 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/codepipeline Issues and PRs that pertain to the codepipeline service. size/L Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

r/aws_codepipeline should support action namespace
9 participants