refactor(elasticsearch): Simplify and remove redundant resources for elasticsearch #12928
Conversation
|
|
|
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
dsharkou
changed the title
dsharkou
changed the title
|
Looks like |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
iliapolo
changed the title
github-actions
bot
added
the
@aws-cdk/aws-elasticsearch
| @@ -1594,6 +1598,9 @@ export class Domain extends DomainBase implements IDomain { | |||
| this.domain = new CfnDomain(this, 'Resource', { | |||
| domainName: this.physicalName, | |||
| elasticsearchVersion, | |||
| accessPolicies: new iam.PolicyDocument({ | |||
There was a problem hiding this comment.
I remember we intentionally didn't use this because of some limitation it presented. @stephanh Do you remember what it was?
There was a problem hiding this comment.
@iliapolo It would be great to understand the meaning (@stephanh). Anyway I tested it with CfnDomain and it was deployed and it's working well.
However I'd like to use aws-elasticsearch lib and I hope we can improve and remove redundant code (piece of CF stack).
One more question - do you have any ETA on marking as stable (right now it has experimental status)?
There was a problem hiding this comment.
I think this is the issue we had around this #8369 (comment).
There was a problem hiding this comment.
@dsharkou So the reason we did this is because we wanted to include the domain ARN in the resource policy.
Something doesn't add up here, since the policy still contains this.domainArn, I would expect the integration test to fail. Are you sure to ran it with the up to date code?
In any case if we can just use * there instead, i'm also good with that. Once you get the build passing, i'm happy to merge this.
Regarding the stability of the module, no ETA just yet, but we are starting to consider it. We will update in the tracking issue.
| : props.accessPolicies; | ||
|
|
||
| if (accessPolicyStatements != null) { | ||
| const accessPolicy = new ElasticsearchAccessPolicy(this, 'ESAccessPolicy', { |
There was a problem hiding this comment.
If you are able to not use ElasticsearchAccessPolicy then I think it would be good to delete that whole custom resource and the corresponding tests.
|
@dsharkou Are you still pursuing this PR? |
iliapolo
added
the
response-requested
|
This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled. |
github-actions
bot
added
closing-soon
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license