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

Bugfix/change config for relational database ds #234

Merged

Conversation

roznalex
Copy link
Contributor

Hi, @bboure. Even AWS documentation may be wrong :)
There is a fix for the issue. I have added an automatic "dbClusterArn" generation. Now it should work correctly.

@roznalex
Copy link
Contributor Author

roznalex commented Apr 1, 2019

So, doc is wrong? https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-appsync-datasource-relationaldatabaseconfig-rdshttpendpointconfig.html#cfn-appsync-datasource-relationaldatabaseconfig-rdshttpendpointconfig-dbclusteridentifier

DbClusterIdentifier expects an arn?

@bboure yes. I tested this behavior within my environment. It only works if you provide full ARN for a cluster.

@@ -104,6 +104,31 @@ class ServerlessAppsyncPlugin {
return { "Fn::GetAtt": [lambdaLogicalId, "Arn"] };
}

getDbClusterArn(config) {
if (config && config.dbClusterIdentifier) {
return this.generateDbClusterArn(config.dbClusterIdentifier, config.region);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@roznalex Could we maybe implement it so that:

  • if an arn is given, use it
  • if an "identifier" is given, generate the Arn from it

I would also keep an eye on Cloudformation's doc. I wouldn't be surprised if they'd "fix it" or add a new key dbClusterArn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bboure sure. So should we add support for two options: dbClusterIdentifier and dbClusterArn?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I would wait and see what AWS does.
On second thought, let's leave it as it is and keep it "in sync" with cloudformations

Copy link
Collaborator

Choose a reason for hiding this comment

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

@roznalex ok, so in fact that means that dbClusterIdentifier should receive an arn, and we could forget about generating it
What do you think?

Copy link
Contributor Author

@roznalex roznalex Apr 1, 2019

Choose a reason for hiding this comment

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

@bboure I'm not sure that it is the right decision, because a user needs to build this ARN by himself. According to documentation Ref function returns a resource name, not ARN.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is the Fn::GetAtt function, but now I see that it does not return the Arn (not sure).
Let's keep it as it is. It would be good to maybe write a post on AWS forum and ask for more info on this.

@bboure
Copy link
Collaborator

bboure commented Apr 11, 2019

@roznalex A little follow up on this.
Sorry for not merging this earlier. I really want to merge this asap, but I am a bit afraid that this could break again in the future.
I opened a question on the AWS forum. I'd like to have feedback from them before doing anything.

@sid88in
Copy link
Owner

sid88in commented Apr 12, 2019

@bboure I can follow up on this internally in AWS. Will sync up with you offline.

@roznalex
Copy link
Contributor Author

@roznalex A little follow up on this.
Sorry for not merging this earlier. I really want to merge this asap, but I am a bit afraid that this could break again in the future.
I opened a question on the AWS forum. I'd like to have feedback from them before doing anything.

Hi @bboure. Are there any updates? Could we merge this PR already?

@bboure
Copy link
Collaborator

bboure commented Apr 24, 2019

Sorry for late response @roznalex
AWS confirmed that it should be an Arn and they will fix the doc.

@bboure bboure merged commit 5f31c1b into sid88in:master Apr 24, 2019
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.

3 participants