-
Notifications
You must be signed in to change notification settings - Fork 818
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(graphql): correct searchable instance types #9973
fix(graphql): correct searchable instance types #9973
Conversation
Why not just remove the |
If we remove the allowed values list and someone misspell an instance type, the error would appear at much later time on push resulting in poor DX. We are thinking about using SDK to retrieve the allowed instance types. |
d5dfd31
to
699db62
Compare
Codecov Report
@@ Coverage Diff @@
## master aws-amplify/amplify-cli#9973 +/- ##
==========================================
- Coverage 54.09% 54.08% -0.01%
==========================================
Files 837 837
Lines 46374 46376 +2
Branches 9890 9891 +1
==========================================
Hits 25084 25084
- Misses 19295 19296 +1
- Partials 1995 1996 +1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
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 q, otherwise LGTM
@@ -157,85 +157,45 @@ export function createParametersStack(stack: Stack): Map<string, CfnParameter> { | |||
'c6g.4xlarge.elasticsearch', | |||
'c6g.xlarge.elasticsearch', | |||
'c6g.12xlarge.elasticsearch', | |||
't3.small.search', |
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.
Is this list the same as the second one down below? can we collapse these down to a single list so we're not duplicating these PRs across two files in the future?
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 list is maintained separately for v1 and v2 transformers. It was intentionally maintained in different places to keep v1 and v2 packages independent of each other. The V1 will eventually go away.
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.
LGTM
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.
LGTM!
@sundersc why were all the I believe
That document goes on to say that
BUT it seems that if this is going to happen, it hasn't yet. |
Fixes #9956