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

[WIP] Add Codepipeline action region support #7866

Conversation

bashton-ajenkins
Copy link
Contributor

This was added in November:

Fixes #6871

Changes proposed in this pull request:

This has been tested and worked for setting up cross region actions on some pipelines that I am working on as part of a project.

Could someone please take a look at the PR and let me know what if anything needs to be done to get this merged in?

Thanks,

Alan Jenkins

@ghost ghost added size/XS Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. labels Mar 8, 2019
@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. service/codepipeline Issues and PRs that pertain to the codepipeline service. labels Mar 8, 2019
@bashton-ajenkins
Copy link
Contributor Author

Please do check what is here currently and let me know if anything needs fixing, however I have since discovered that this commit in isolation is not that helpful due to needing to be able to specify multiple ArtifactStores (one for each region used in the pipeline) as can be seen documented here:

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-codepipeline-pipeline.html#cfn-codepipeline-pipeline-artifactstores

I am currently working on an additional commit to add artifact_stores.

@alexvanbelle
Copy link

What is the status with this PR? Would really appreciate it :)

@bashton-ajenkins
Copy link
Contributor Author

bashton-ajenkins commented May 27, 2019 via email

@alexvanbelle
Copy link

Unfortunately I am stalled on it at current as having to work on other things. I am going to try and pick it up again in the coming weeks though.

On Mon, May 27, 2019, 16:38 Alexandre Vanbelle @.***> wrote: What is the status with this PR? Would really appreciate it :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7866?email_source=notifications&email_token=AGK7IJOQNTMTXFOTEMYUOVTPXP56BA5CNFSM4G4ULX6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWKCX4Q#issuecomment-496249842>, or mute the thread https://github.com/notifications/unsubscribe-auth/AGK7IJJ2RFVLHLEKDEMTRKTPXP56BANCNFSM4G4ULX6A .

Great! Thank you. We really need it for some pipelines.

@aeschright aeschright requested a review from a team June 26, 2019 00:52
Copy link

@arvinep arvinep left a comment

Choose a reason for hiding this comment

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

LGTM

@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Jul 28, 2019
@rkreich
Copy link

rkreich commented Aug 1, 2019

Has anyone found a workaround until this PR gets merged?

@andgara
Copy link

andgara commented Aug 9, 2019

Any chance to get this merged soon? I managed to fix the build by replacing Required: false with Optional: true.

@bashton-ajenkins
Copy link
Contributor Author

@rkreich my workaround was to use https://www.terraform.io/docs/providers/aws/r/cloudformation_stack.html to wrap Cloudformation for defining the multi region Codepipeline.

@bashton-ajenkins
Copy link
Contributor Author

@andgara unfortunately while I can make it compile I have not managed to make it work as yet. The closest I have got to make it work is where Terraform attempts to plan but then crashes and this is pretty much where I am at at the moment. I am finding it very hard to debug the crashes due to the AWS provider running as a separate process to Terraform so I cannot easily use a debugger (delve) or get any debug output.

@kwyjibos
Copy link

@bashton-ajenkins Did you get any further with this? It looks like it could be a useful feature

@bashton-ajenkins
Copy link
Contributor Author

@kwyjibos unfortunately not. Things have been busy at work and I haven't had chance to look at it. The state is that it can compile but I cannot seem to get the actifact_stores data structure right for Terraform to work with it.

This is made harder to debug by the fact that I cannot seem to get any kind of debug output out of the AWS provider. (I have attempted to write what is passed to the AwsCodePipeline resource to file and tried several ways to print the variables but cannot get any output) (Suggestions very much welcome on how to debug this for when I get time to attempt finishing this).

@nikoe14
Copy link

nikoe14 commented Sep 19, 2019

Hey! we are currently blocked for this... :(

@Israphel
Copy link

is the cross-region ever going to be supported or not? Everytime something like this happens, Hashicorp suggest to use Cloudformation as a workaround. Then I ask myself, why am I using Terraform then? I even pay for it, and I get this kind of answers.

@aeschright
Copy link
Contributor

Hi everyone! The Terraform AWS Provider team has limited resources to review PRs, so we do look to community members to move things past the WIP state if possible. If another person on the thread has found a fix that works, you could submit a new PR for it. Also, if you're a paid customer, we would be better able to prioritize these changes if you do a support request: https://support.hashicorp.com

Thanks for digging into this, we appreciate the work.

@nikoe14
Copy link

nikoe14 commented Oct 2, 2019

I am a paid customer and I already filled a ticket to support, one of the answers I got was to use Cloudformation like a workaround... So Terraform team pointing me to use "Cloudformation", my team had to migrate the code from Terraform to Cloudformation and create new Terraform modules to run the CF templates, and the funny thing is that after this fix all that code will be discarded, this workaround was very expensive in terms of work...

@aeschright aeschright self-assigned this Oct 7, 2019
@aeschright
Copy link
Contributor

Quick update: we're doing some research on the best way to handle this within Terraform, and I'll post again when I have more.

@aeschright aeschright removed their assignment Oct 28, 2019
@jferg-newcontext
Copy link

It's running into this kind of issue that makes me regret starting down the road of using terraform in a project. It completely blocks doing certain types of functionality, has been open for almost a year, hasn't been touched for almost four months, and the workaround is "use CloudFormation" (which might as well be "don't use terraform"). sigh

I would take a look at the code, but the last time I submitted a terraform-providers PR, it sat un-looked at by anyone for 6+ months, by which time the code was stale and couldn't be merged, so it got closed as non-working. Oh, well. Off to learn CloudFormation, I guess.

Copy link
Contributor

@gdavison gdavison left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @bashton-ajenkins, it's a great start. I apologize for us leaving this for such a long time without response. We can work with you to get the PR merged, or if you'd prefer, we can finish it up.

I've added a number of changes to make. We would also require acceptance tests and some expanded documentation before accepting the PR.

aws/resource_aws_codepipeline.go Outdated Show resolved Hide resolved
aws/resource_aws_codepipeline.go Show resolved Hide resolved
aws/resource_aws_codepipeline.go Outdated Show resolved Hide resolved
aws/resource_aws_codepipeline.go Outdated Show resolved Hide resolved
aws/resource_aws_codepipeline.go Outdated Show resolved Hide resolved
aws/resource_aws_codepipeline.go Outdated Show resolved Hide resolved
@gdavison gdavison self-assigned this Feb 19, 2020
@JWasdin
Copy link

JWasdin commented Feb 19, 2020

Happy to see a response and some progress here! Onward friends..

@alisonjenkins
Copy link

@gdavidson as mentioned in one of my previous messages I was finding this hard to debug and couldn't understand why the provider was crashing. I have pretty much been snowed under with work since then and not had chance to look at it. I have the next 4 days off though so if you are up for it I would very much like to work with you to get it finished off (I am keen to learn where I went wrong and how to debug the provider if I am stuck in this situation again as I intend to contribute more things to the provider).

@alisonjenkins
Copy link

Also note that @bashton-ajenkins is my work account @alanjjenkins is my personal

Adds support for specifying Artifact Stores to enable multi region
pipelines.
@bashton-ajenkins bashton-ajenkins force-pushed the ajj/add-codepipeline-action-region-support branch from a97f8c2 to 8ec376a Compare February 19, 2020 23:24
@ghost ghost added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Feb 20, 2020
@bashton-ajenkins bashton-ajenkins force-pushed the ajj/add-codepipeline-action-region-support branch from ddfbb9a to 4162fc2 Compare February 23, 2020 20:05
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Feb 23, 2020
@gdavison
Copy link
Contributor

gdavison commented Mar 9, 2020

Hi @bashton-ajenkins / @alanjjenkins ,

I've done a bit of debugging on this.

Firstly, can you rebase the PR onto the current master branch of the provider? We've made some internal changes to the provider, the most important one being moving the Terraform resource code to its own repo https://github.com/hashicorp/terraform-plugin-sdk.

It's kinda hard to decipher it in the crash output, but I've found a few things. Here's the line that identifies the problem, out of almost 60 lines of output :)

panic: Error reading level config: '' expected type 'string', got unconvertible type 'map[string]interface {}'

Basically, the resource has mismatching types. In this case, it's because we don't actually support complex element types for maps (I just learned this). So we'll have to use a different data structure. I did also notice that the Elem in artifact_store is missing the nested &schema.Resource{.

A TypeSet would be the best in this case, since we don't need any ordering. The region argument will have to be made part of the set element.

We have an undocumented practice of using singular argument names rather than plural. artifact_store collides with the existing argument, but with some additional logic, we should be able to use both the singular and multiple cases. The region argument would only be required in the multiple cases, and possibly not allowed in the single case.

For example:

resource "aws_codepipeline" "singular" {
  artifact_store {
    location = aws_s3_bucket.singular.bucket
    type     = "S3"
  }
}
resource "aws_codepipeline" "multiple" {
  artifact_store {
    region = var.region1
    location = aws_s3_bucket.here.bucket
    type     = "S3"
  }
  artifact_store {
    region = var.region2
    location = aws_s3_bucket.there.bucket
    type     = "S3"
  }
}

This shouldn't affect existing state, so won't be a breaking change.

Things that will need to be tested for:

  • Switching from single artifact stores to multiple and vice versa
  • If CodePipeline supports it, a single artifact store but in a different region

To test the multiple region cases, you'll need to create multiple AWS provider blocks and alias them (https://www.terraform.io/docs/configuration/providers.html#alias-multiple-provider-instances) and the resources created in the other regions will have to have the appropriate provider argument.

This can be done by adding testAccAlternateRegionProviderConfig() to your test configuration and use the alternate provider alias.

resource "aws_s3_bucket" "here" {
  bucket = "tf-test-pipeline-here-%[1]s"
  acl    = "private"
}

resource "aws_s3_bucket" "there" {
  provider = aws.alternate
  bucket   = "tf-test-pipeline-there-%[1]s"
  acl      = "private"
}

And call it with

fmt.Sprintf(yourTerraformCode, rName) + testAccAlternateRegionProviderConfig()

Also update the PreCheck field on the test case to

PreCheck: func() {
	testAccPreCheck(t)
	testAccMultipleRegionsPreCheck(t)
	testAccAlternateRegionPreCheck(t)
},

@Israphel
Copy link

is this going somewhere or not?

@gdavison
Copy link
Contributor

I've created #12549 to complete this feature

@bflad
Copy link
Contributor

bflad commented Apr 3, 2020

This was merged in as part of #12549, but GitHub was having issues at the time it was merged so was likely not automatically closed due to that. Closing manually.

@bflad bflad closed this Apr 3, 2020
@bflad bflad added this to the v2.56.0 milestone Apr 3, 2020
@ghost
Copy link

ghost commented Apr 3, 2020

This has been released in version 2.56.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 3, 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 3, 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/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
Development

Successfully merging this pull request may close these issues.

Cross Region Pipelines