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

fix: Basic authentication configuration at the branch level #15

Merged

Conversation

sestrella
Copy link
Contributor

what

Module consumers cannot pass a custom basic_auth_credentials per branch using the existing variables. At the branch level, there is also a typo on enable_basic_auth.

why

The changes made in this PR allow consumers to configure different basic authentication credentials for each branch.

@sestrella sestrella requested review from a team as code owners October 16, 2023 20:38
sadjow

This comment was marked as outdated.

@skilef
Copy link

skilef commented Nov 1, 2023

Please review this, it would be really helpful!

@kevcube
Copy link
Contributor

kevcube commented Dec 27, 2023

@sestrella can you please update base branch and I will look to get this merged

@kevcube kevcube self-assigned this Dec 27, 2023
@kevcube
Copy link
Contributor

kevcube commented Dec 28, 2023

/terratest

@kevcube
Copy link
Contributor

kevcube commented Dec 28, 2023

@sestrella tests failing and readme is out of date, can you locally run make init; make readme (on macOS brew install make; gmake init; gmake readme)

@sestrella sestrella force-pushed the pass_basic_auth_credentials_to_branches branch from 3841159 to a8230fd Compare December 29, 2023 01:00
@sestrella
Copy link
Contributor Author

@kevcube done. Would you mind taking a second look, please?

@kevcube
Copy link
Contributor

kevcube commented Dec 29, 2023

/terratest

@kevcube
Copy link
Contributor

kevcube commented Dec 30, 2023

@sestrella tests are still failing. It doesn't appear to be related to your PR but if you have time to investigate that would be appreciated. Otherwise I'll check it out when I'm able to.

@sestrella
Copy link
Contributor Author

sestrella commented Jan 3, 2024

@kevcube After taking a closer look at the job log, I found the following relevant information:

TestExamplesComplete 2023-12-29T06:54:08Z logger.go:66: │ Error: creating Amplify App (eg-ue2-test-amplify-jrjxrn): BadRequestException: There was an issue setting up your repository. Please try again later.({"message":"Moved Permanently","url":"https://api.github.com/repositories/498423879/hooks","documentation_url":"https://docs.github.com/rest/guides/best-practices-for-using-the-rest-api#follow-redirects"})
TestExamplesComplete 2023-12-29T06:54:08Z logger.go:66: │ 
TestExamplesComplete 2023-12-29T06:54:08Z logger.go:66: │   with module.amplify_app.aws_amplify_app.default[0],
TestExamplesComplete 2023-12-29T06:54:08Z logger.go:66: │   on ../../main.tf line 10, in resource "aws_amplify_app" "default":
TestExamplesComplete 2023-12-29T06:54:08Z logger.go:66: │   10: resource "aws_amplify_app" "default" {
TestExamplesComplete 2023-12-29T06:54:08Z logger.go:66: │ 
TestExamplesComplete 2023-12-29T06:54:08Z logger.go:66: ╵
TestExamplesComplete 2023-12-29T06:54:08Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: exit status 1; ╷
│ Error: creating Amplify App (eg-ue2-test-amplify-jrjxrn): BadRequestException: There was an issue setting up your repository. Please try again later.({"message":"Moved Permanently","url":"https://api.github.com/repositories/498423879/hooks","documentation_url":"https://docs.github.com/rest/guides/best-practices-for-using-the-rest-api#follow-redirects"})

I'm wondering if this is related to the fact that the amplify-test2 appears to be transferred from the cloudposse organization to the cloudposse-archives organization. I submitted a commit updating the URL for the repository found in the complete example.

@kevcube
Copy link
Contributor

kevcube commented Jan 3, 2024

/terratest

@sestrella
Copy link
Contributor Author

@kevcube I got a different error this time. Perhaps is related to the permissions associated with the GH token?

TestExamplesComplete 2024-01-03T03:18:06Z logger.go:66: │ Error: creating Amplify App (eg-ue2-test-amplify-t9vp6d): BadRequestException: There was an issue setting up your repository. Please try again later.({"message":"Not Found","documentation_url":"https://docs.github.com/rest/repos/webhooks#create-a-repository-webhook"})
TestExamplesComplete 2024-01-03T03:18:06Z logger.go:66: │ 
TestExamplesComplete 2024-01-03T03:18:06Z logger.go:66: │   with module.amplify_app.aws_amplify_app.default[0],
TestExamplesComplete 2024-01-03T03:18:06Z logger.go:66: │   on ../../main.tf line 10, in resource "aws_amplify_app" "default":
TestExamplesComplete 2024-01-03T03:18:06Z logger.go:66: │   10: resource "aws_amplify_app" "default" {
TestExamplesComplete 2024-01-03T03:18:06Z logger.go:66: │ 
TestExamplesComplete 2024-01-03T03:18:06Z logger.go:66: ╵
TestExamplesComplete 2024-01-03T03:18:06Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: exit status 1; ╷
│ Error: creating Amplify App (eg-ue2-test-amplify-t9vp6d): BadRequestException: There was an issue setting up your repository. Please try again later.({"message":"Not Found","documentation_url":"https://docs.github.com/rest/repos/webhooks#create-a-repository-webhook"})
│ 
│   with module.amplify_app.aws_amplify_app.default[0],
│   on ../../main.tf line 10, in resource "aws_amplify_app" "default":
│   10: resource "aws_amplify_app" "default" {
│ 
╵}

@kevcube
Copy link
Contributor

kevcube commented Jan 3, 2024

@sestrella not sure how to proceed with that one. for a shorter debug loop you can try just terraform apply-ing the examples/complete into your own AWS and GitHub accounts until you find a working solution. I'm not an admin in Cloudposse so if it's a token issue I can call in someone else.

@sestrella
Copy link
Contributor Author

@kevcube I finally made it back to this PR. I made it work by creating a fork of https://github.com/cloudposse-archives/amplify-test2 to which I had write access; the following GH token permissions were required to make the complete/example work:

image

On a side note, according to the official documentation, a token with write access is required to set up a webhook, so my guess is that the token used by the CI lacks the necessary permissions:

image

Reference: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/amplify_app#access_token

@sestrella
Copy link
Contributor Author

Hi @kevcube, I'm wondering if there is anything else I can do on my end to help get this PR move forward. Please let me know.

@hans-d
Copy link

hans-d commented Mar 2, 2024

/terratest

@hans-d hans-d added wip Work in Progress: Not ready for final review or merge and removed wip Work in Progress: Not ready for final review or merge labels Mar 2, 2024
@hans-d
Copy link

hans-d commented Mar 2, 2024

/terratest

@sestrella
Copy link
Contributor Author

@kevcube Thank you for triggering the CI again. I realized the check for the README was failing, so I re-ran gmake init; gmake readme.

@kevcube
Copy link
Contributor

kevcube commented Mar 4, 2024

/terratest

@sestrella
Copy link
Contributor Author

I look closely at the error message:

BadRequestException: There was an issue setting up your repository. Please try again later

However, it appears to be a little misleading because the GH status page has not revealed any recent issues. Based on my tests a few weeks ago, I continue to believe this is related to token permissions. I'm wondering if the token was revisited.

@kevcube
Copy link
Contributor

kevcube commented Mar 4, 2024

@sestrella the repo has been moved from cloudposse-archives back to cloudposse, can you please update the reference in this PR and we will try tests again

@sestrella
Copy link
Contributor Author

@kevcube done! Let's see how it goes 🤞🏼

@kevcube kevcube added minor New features that do not break anything and removed triage Needs triage labels Apr 30, 2024
Copy link

mergify bot commented Apr 30, 2024

Important

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

@sestrella sestrella force-pushed the pass_basic_auth_credentials_to_branches branch from 2cc5002 to 4738290 Compare May 1, 2024 15:51
@goruha
Copy link
Member

goruha commented May 2, 2024

/terratest

@sestrella sestrella changed the title Fix basic authentication configuration at the branch level fix: Basic authentication configuration at the branch level May 2, 2024
@sestrella
Copy link
Contributor Author

@goruha terratest appears to be failing for the same reasons that it fails on #32. Here are the links to the runs:

image

@osterman
Copy link
Member

osterman commented May 3, 2024

We have some work here to fix the tests. I cannot yet commit to an ETA. 🙏 for your understanding. We are tracking it internally.

@goruha
Copy link
Member

goruha commented Jun 21, 2024

/terratest

@fm7-1 fm7-1 force-pushed the pass_basic_auth_credentials_to_branches branch from 4738290 to 8a7de96 Compare June 21, 2024 17:38
@fm7-1
Copy link
Contributor

fm7-1 commented Jun 22, 2024

Hello, @goruha. I have rebased this PR with the latest commits in main. Could you run the tests again, please?

@osterman
Copy link
Member

/terratest

@fm7-1
Copy link
Contributor

fm7-1 commented Jul 6, 2024

Hello, everyone! I see terratest is still failing with the same error as before. Is there anyway we can assist to move forward with the fix?

@osterman
Copy link
Member

osterman commented Jul 6, 2024

The hold up is we finally had switched/deprecated all our workflows in hundreds of repos from using PATs, but now this one repo needs a PAT. The terraform provider for amplify doesn't work with fine grained tokens. As a result we need to do a one off for this repo. We are still working on it, but it's in between other work.

@fm7-1
Copy link
Contributor

fm7-1 commented Jul 6, 2024

Got it! Thank you for your prompt response, @osterman!

@goruha
Copy link
Member

goruha commented Jul 15, 2024

@sestrella can you pls update branch or rebase?

@fm7-1 fm7-1 force-pushed the pass_basic_auth_credentials_to_branches branch from 8a7de96 to a54107b Compare July 15, 2024 15:59
@fm7-1
Copy link
Contributor

fm7-1 commented Jul 15, 2024

@goruha Done, I have rebased this branch

@goruha
Copy link
Member

goruha commented Jul 15, 2024

/terratest

@goruha goruha merged commit 6277327 into cloudposse:main Jul 15, 2024
15 checks passed
@mergify mergify bot removed the needs-cloudposse Needs Cloud Posse assistance label Jul 15, 2024
Copy link

These changes were released in v1.1.0.

@sestrella sestrella deleted the pass_basic_auth_credentials_to_branches branch July 15, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor New features that do not break anything
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants