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-searchable-transformer): formatting domain name #1916

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

Conversation

snsarij
Copy link

@snsarij snsarij commented Oct 4, 2023

Description of changes

  • Created a new resource helper function that will generate the domain name for open search domain
  • Referred to this function in the CDK
  • Completed the local testing
CDK / CloudFormation Parameters Changed

No parameters have been changed

Issue #, if available

Solves the issue: #907

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Any CDK or CloudFormation parameter changes are called out explicitly

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@snsarij snsarij requested review from a team as code owners October 4, 2023 11:52
@dpilch
Copy link
Member

dpilch commented Oct 10, 2023

I think the change makes sense to me. I'll need to investigate if this could have ill effects on existing apps.

There is a failing test on the mock util. cd packages/amplify-util-mock then yarn e2e to see the failure.

Also, can you re-run yarn extract-api? Seems to just be a formatting issue.

@AaronZyLee
Copy link
Contributor

The domain name is an attribute which requires replacement, during which process the new search domain with the given name is generated and the old one being deleted as follow up. This is definitely a breaking change for the existing users. Thus this PR will be held until a migration plan is figured out.

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-opensearchservice-domain.html#cfn-opensearchservice-domain-domainname

As a workaround for the linked issue, the amplify override api can be used to change the naming of search domain. The example override.ts is as following.

import { AmplifyApiGraphQlResourceStackTemplate, AmplifyProjectInfo } from '@aws-amplify/cli-extensibility-helper';

export function override(resources: AmplifyApiGraphQlResourceStackTemplate, amplifyProjectInfo: AmplifyProjectInfo) {
  resources.opensearch.OpenSearchDomain.domainName = `amplify-opensearch-${amplifyProjectInfo.envName}`;
}

@@ -29,7 +28,7 @@ export const createSearchableDomain = (
zoneAwareness: {
enabled: false,
},
domainName: Fn.conditionIf(HasEnvironmentParameter, Fn.ref('AWS::NoValue'), `d${apiId}`).toString(),
domainName: context.resourceHelper.generateDomainName(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the domain name may be destructive for the existing apps. We need to verify that this change result in loss of data.

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.

4 participants