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] env AWS_REGION #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[add] env AWS_REGION #7

wants to merge 2 commits into from

Conversation

parpa
Copy link

@parpa parpa commented May 18, 2021

@parpa parpa changed the title Wip: [add] env AWS_REGION [add] env AWS_REGION May 18, 2021
@parpa parpa changed the title [add] env AWS_REGION Wip: [add] env AWS_REGION May 18, 2021
@parpa parpa changed the title Wip: [add] env AWS_REGION [add] env AWS_REGION May 18, 2021
@parpa
Copy link
Author

parpa commented May 19, 2021

Hello

This PR is related to #5 And fix AWS_REGION bug which COGNITO_USER_POOL_ID should be the same AWS_REGION.

@br4in3x please let me know if it's ok with you or if I should change anything.

Best wishes

Copy link
Owner

@max-pv max-pv left a comment

Choose a reason for hiding this comment

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

Sorry for the long reply, and thanks for the contribution! Have 1 question:

```

With client secret:

```go
AWS_PROFILE=XXX COGNITO_APP_CLIENT_ID=XXX COGNITO_APP_CLIENT_SECRET=XXX COGNITO_USER_POOL_ID=XXX PORT=8080 ./build/cognito
AWS_REGION=XXX COGNITO_APP_CLIENT_ID=XXX COGNITO_APP_CLIENT_SECRET=XXX COGNITO_USER_POOL_ID=XXX PORT=8080 ./build/cognito
Copy link
Owner

Choose a reason for hiding this comment

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

I think we still need AWS_PROFILE along with AWS_REGION change. Sometimes there are more profiles than one default, so that's why it was here. Could you please explain why did you remove it?

Copy link
Author

@parpa parpa Jun 7, 2021

Choose a reason for hiding this comment

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

@br4in3x Thank you!
I use docker without setting the configuration file (~/.aws/credentials), we have already set COGNITO_APP_CLIENT_ID=XXX COGNITO_USER_POOL_ID=XXX only need to set another AWS_REGION to execute all APIs.

https://github.com/br4in3x/golang-cognito-example/pull/7/files#diff-2873f79a86c0d8b3335cd7731b0ecf7dd4301eb19a82ef7a1cba7589b5252261L55-R55

Because the Region is fixed in the main, I still can't debug successfully. I found that only AWS_REGION is required.

Is AWS_PROFILE necessary for your development environment?

Copy link
Owner

Choose a reason for hiding this comment

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

The purpose of this repo is to provide the simplest possible example of how to use Cognito in Go, therefore AWS_PROFILE is needed. I think that docker is a bit of an advance technique and might confuse other devs who just want to check out how this repo works. Because of this, I propose to have both AWS_PROFILE and AWS_REGION in the README, like this:

AWS_PROFILE=XXX AWS_REGION=XXX COGNITO_APP_CLIENT_ID=XXX COGNITO_APP_CLIENT_SECRET=XXX  COGNITO_USER_POOL_ID=XXX PORT=8080 ./build/cognito

@@ -28,6 +28,12 @@ func (a *App) Register(w http.ResponseWriter, r *http.Request) {
},
}

// Compute secret hash based on client secret.
if a.AppClientSecret != "" {

Choose a reason for hiding this comment

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

The a.AppClientSecret is not empty when we reach this if. Code inside this branch is not executed, you should just remove the if branch and the app will work perfectly.

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.

Secret hash is missing for register
3 participants