-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Appsync datasource none #7145
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
Appsync datasource none #7145
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
/** | ||
* Properties for an AppSync dummy datasource | ||
*/ | ||
export interface NoneDataSourceProps extends BaseDataSourceProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eladb whats your opinion on extending just for renaming like this? Is it appropriate for NoneDateSource
to just accept BaseDataSourceProps
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this datasource wouldn't allow you to set serviceRole at all.
My hacky workaround to this in #6836 (comment) was to do:
export interface NoneDataSourceProps extends BaseDataSourceProps {
readonly serviceRole?: undefined
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way to implement this would be to remove serviceRole from BaseDataSourceProps, and instead add it to a new BackedDataSourceProps interface that others can inherit from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like something we should do sooner or later. @christophgysin you okay with implementing that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will try to do this early next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have extracted serviceRole
to BackedDataSourceProps
. Please have a look at 154390d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question still remains if NoneDateSourceProps
should exist at all, or if NoneDateSource
should just accept BaseDataSourceProps
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep NoneDataSourceProps for now.
77e7f3c
to
01f0670
Compare
01f0670
to
ffdba44
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Commit Message
feat: Support AppSync DataSource type: NONE
End Commit Message
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license