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

Remove CodePipeline GitHub environment variable #14175

Merged
merged 10 commits into from
Jul 31, 2020

Conversation

gdavison
Copy link
Contributor

@gdavison gdavison commented Jul 14, 2020

Removes the previously required GITHUB_TOKEN environment variable for CodePipeline. The authentication token is now passed as the OAuthToken field in the action configuration.

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 #2796
Closes #4768

Release note for CHANGELOG:

resource/aws_codepipeline: Removes `GITHUB_TOKEN` environment variable

Output from acceptance testing in commercial partition:

$ make testacc TESTARGS='-run=TestAccAWSCodePipeline_'

--- PASS: TestAccAWSCodePipeline_disappears (35.93s)
--- PASS: TestAccAWSCodePipeline_emptyStageArtifacts (40.89s)
--- PASS: TestAccAWSCodePipeline_multiregion_basic (48.58s)
--- PASS: TestAccAWSCodePipeline_deployWithServiceRole (55.08s)
--- PASS: TestAccAWSCodePipeline_WithNamespace (58.69s)
--- PASS: TestAccAWSCodePipeline_basic (63.80s)
--- PASS: TestAccAWSCodePipeline_tags (75.56s)
--- PASS: TestAccAWSCodePipeline_multiregion_Update (79.32s)
--- PASS: TestAccAWSCodePipeline_multiregion_ConvertSingleRegion (94.63s)

Output from acceptance testing in GovCloud partition:

--- SKIP: TestAccAWSCodePipeline_multiregion_ConvertSingleRegion (2.37s)
--- SKIP: TestAccAWSCodePipeline_multiregion_basic (2.38s)
--- SKIP: TestAccAWSCodePipeline_multiregion_Update (2.39s)
--- PASS: TestAccAWSCodePipeline_disappears (24.64s)
--- PASS: TestAccAWSCodePipeline_emptyStageArtifacts (30.55s)
--- PASS: TestAccAWSCodePipeline_WithNamespace (32.72s)
--- PASS: TestAccAWSCodePipeline_deployWithServiceRole (35.85s)
--- PASS: TestAccAWSCodePipeline_basic (46.98s)
--- PASS: TestAccAWSCodePipeline_tags (57.72s)

@gdavison gdavison requested a review from a team July 14, 2020 19:36
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. service/codepipeline Issues and PRs that pertain to the codepipeline service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jul 14, 2020
@@ -488,10 +592,29 @@ func testAccPreCheckAWSCodePipeline(t *testing.T) {
}
}

func testAccPreCheckAWSCodePipelineAlternateRegion(t *testing.T) {
// There isn't a way to get the alternate region provider at PreCheck time, so hardcode it
if testAccGetAlternateRegion() == "us-gov-east-1" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, but this might be better as a check on testAccGetAlternateRegionPartition() == "aws-us-gov" instead…

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow this is annoying (on the AWS side), I'm fine with either, to be honest.

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.

This is well implemented so far given the existing Terraform core and SDK constraints, but I see what you are running into regarding that attribute since its TypeMap. 😬 I'm guessing we will want to circle the wagons and come up with an overall plan for this, even if that means (hopefully temporarily) punting on this as part of the current major version release.

aws/resource_aws_codepipeline.go Show resolved Hide resolved

const codePipelineGitHubTokenHashPrefix = "hash-"

func hashCodePipelineGitHubToken(token string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

We are actively in the process of removing the few other instances of hashed state storage, however Terraform in general doesn't handle TypeMap and Sensitive: true in a meaningful way without making the entire map sensitive, if I remember correctly. 😖

It would be a bummer to introduce this technical debt in, but otherwise we could also be forced down the road of what I think you were suggesting the other day of potentially going against the API design slightly and implementing bespoke Terraform schema so we can control it better (and in this situation, actually implement a single sensitive attribute). We do have some slight precedence of this class of user experience enhancement with resources like aws_sns_platform_application and I believe aws_sns_topic.


I think this leaves us in a tough spot. 🙁 We can try to quickly design something better for this situation to squeeze it in for this major release in the next two weeks, which is fairly risky in my opinion, or punt on this for now and spend later cycles on this issue in general (potentially changing the Core/SDK sensitive map abilities and/or ensuring we get good feedback on a new resource design). What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the environment variable is a huge pain-point for practitioners, and waiting until 4.0 to remove it would be less than ideal. If we don't want to hash the value, would setting the TypeMap to Sensitive be a better option at this point? And then we could come up with a better solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our potential path forward will be:

  • Removing the state hashing handling
  • Marking the configuration attribute Sensitive: true and Computed: true
  • Adding a new github/github_configuration configuration block so we can properly show a difference with only the oauth attribute sensitive

These will require apply-time validation to ensure they aren't both defined since ConflictsWith does not support relative references. The other configuration blocks will need to be filled in later due to time. The documentation and upgrade guide should recommend using the new configuration block(s) over the map.

@@ -488,10 +592,29 @@ func testAccPreCheckAWSCodePipeline(t *testing.T) {
}
}

func testAccPreCheckAWSCodePipelineAlternateRegion(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah its a bummer we cannot reuse logic similar to testAccPreCheckAWSCodePipeline here (using a real API call to automatically enable/skip the test), but I see the problem is that we don't initialize an "alternate" global instance of the provider in the testing similar to testAccProvider. I think this is okay as-is for now until we can figure out things as part of #8983 / #8316

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I briefly considered the option of rolling my own connection to test the API, but I figured the hard-coded option was quicker. Though it actually might not be that complicated

@@ -488,10 +592,29 @@ func testAccPreCheckAWSCodePipeline(t *testing.T) {
}
}

func testAccPreCheckAWSCodePipelineAlternateRegion(t *testing.T) {
// There isn't a way to get the alternate region provider at PreCheck time, so hardcode it
if testAccGetAlternateRegion() == "us-gov-east-1" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow this is annoying (on the AWS side), I'm fine with either, to be honest.

@bflad
Copy link
Contributor

bflad commented Jul 30, 2020

Once these pending items are commented/addressed, will provide another review:

  • Fixing merge conflicts
  • Adding a new github/github_configuration configuration block so we can properly show a difference with only the oauth attribute sensitive

@gdavison gdavison force-pushed the f-remove-codepipeline-github-envvar branch from d8a72a8 to 2f0704c Compare July 31, 2020 00:25
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.

LGTM 🚀

@gdavison gdavison merged commit 25cb068 into master Jul 31, 2020
@gdavison gdavison deleted the f-remove-codepipeline-github-envvar branch July 31, 2020 01:27
gdavison added a commit that referenced this pull request Jul 31, 2020
@ghost
Copy link

ghost commented Jul 31, 2020

This has been released in version 3.0.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 Aug 30, 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 Aug 30, 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. service/codepipeline Issues and PRs that pertain to the codepipeline service. size/XL 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
2 participants