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(graphql): add new open search instance types to the allowed list #9834

Merged
merged 1 commit into from
Feb 25, 2022

Conversation

sundersc
Copy link
Contributor

Add OpenSearch instance types to the allowed list.

@sundersc sundersc requested a review from a team as a code owner February 22, 2022 20:40
@sundersc sundersc linked an issue Feb 22, 2022 that may be closed by this pull request
5 tasks
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM as a first step to get customers unblocked.

Two notes:

  • I'm not sure if we should continue investing time making changes for the v1 transformer.
  • If I recall correctly, not all instance types are supported in all regions. I think we need to make the list here contingent on the region. No need to make that change in this PR though.

Copy link
Contributor

@danielleadams danielleadams left a comment

Choose a reason for hiding this comment

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

Agree with @cjihrig about not investing in V1, but 👍🏼 .

@codecov-commenter
Copy link

Codecov Report

Merging #9834 (6de42b6) into master (fcf9100) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9834   +/-   ##
=======================================
  Coverage   53.10%   53.10%           
=======================================
  Files         830      830           
  Lines       45991    45991           
  Branches     9820     9820           
=======================================
  Hits        24423    24423           
  Misses      19556    19556           
  Partials     2012     2012           
Impacted Files Coverage Δ
...chable-transformer/src/cdk/create-cfnParameters.ts 100.00% <ø> (ø)
...graphql-elasticsearch-transformer/src/resources.ts 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcf9100...6de42b6. Read the comment docs.

@sundersc
Copy link
Contributor Author

LGTM as a first step to get customers unblocked.

Two notes:

  • I'm not sure if we should continue investing time making changes for the v1 transformer.
  • If I recall correctly, not all instance types are supported in all regions. I think we need to make the list here contingent on the region. No need to make that change in this PR though.

Agreed. We need to have a map to store the allowed instance types based on the region.
The reason for adding the new instance types to the V1 transformer is to unblock the customer.

@sachscode sachscode merged commit 39014c3 into aws-amplify:master Feb 25, 2022
@github-actions github-actions bot added the referenced-in-release Issues referenced in a published release changelog label Mar 7, 2022
@github-actions
Copy link

github-actions bot commented Mar 7, 2022

👋 Hi, this pull request was referenced in the v7.6.23 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v7.6.23.

@michaetto
Copy link

michaetto commented Mar 11, 2022

Using new types (i.e. *.search) does not work. Most likely because amplify generates AWS::Elasticsearch::Domain in searchable stack instead AWS::OpenSearchService::Domain.
It would be pretty great to have generated AWS::OpenSearchService::Domain instead, at least under some amplify feature flag in cli.json. We are very close to go to production after hard work and I suppose it could be not easy to change that on production system unless CloudFormation itself allow some seamless migration. Anyway I do elasticsearchVersion = "OpenSearch_1.1" in override.ts, that works with AWS::Elasticsearch::Domain, but only with old instance names, i.e. *.elasticsearch.
@sundersc @cjihrig @danielleadams

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
referenced-in-release Issues referenced in a published release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'ElasticSearchInstanceType' must be one of AllowedValues
6 participants