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(opensearchservice): imported domain's domainendpoint is a url not an endpoint #18027

Merged
merged 8 commits into from Dec 21, 2021

Conversation

joejegan
Copy link
Contributor

@joejegan joejegan commented Dec 15, 2021

imported Domain's domainEndpoint should be a endpoint and not a url.
Fixes #18017

BREAKING CHANGE: imported domain property domainEndpoint used to contain https:// prefix, now the prefix is dropped and it returns the same value as a domainEndpoint on a created domain


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

@gitpod-io
Copy link

gitpod-io bot commented Dec 15, 2021

@github-actions github-actions bot added the @aws-cdk/aws-elasticsearch Related to Amazon Elasticsearch Service label Dec 15, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 15, 2021

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@joejegan joejegan changed the title fix(aws-elasticsearch):imported Domain's domainEndpoint should be a endpoint fix(aws-elasticsearch): imported domain's domainendpoint should be a endpoint Dec 15, 2021
kaizencc
kaizencc previously approved these changes Dec 17, 2021
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Looks good @joejegan! Just a minor nit that I added in

@kaizencc kaizencc changed the title fix(aws-elasticsearch): imported domain's domainendpoint should be a endpoint fix(opensearch): imported domain's domainendpoint is a url not an endpoint Dec 17, 2021
@github-actions github-actions bot added the @aws-cdk/aws-opensearch Related to the @aws-cdk/aws-opensearchservice package label Dec 17, 2021
@mergify mergify bot dismissed kaizencc’s stale review December 17, 2021 04:21

Pull request has been modified.

@kaizencc
Copy link
Contributor

Whoops. I'm glad this didn't get merged because this is a breaking change to a stable module, which is not allowed.

Users may be using domainEndpoint with https:// in their CDK stacks somehow, and they will be broken by this change.

This may be an allowed breaking change, but at the very least we'd have to document it. Give me a second to figure it out.

kaizencc
kaizencc previously approved these changes Dec 20, 2021
@kaizencc kaizencc changed the title fix(opensearch): imported domain's domainendpoint is a url not an endpoint fix(opensearchservice): imported domain's domainendpoint is a url not an endpoint Dec 20, 2021
@mergify mergify bot dismissed kaizencc’s stale review December 20, 2021 18:08

Pull request has been modified.

@kaizencc kaizencc added the pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version. label Dec 20, 2021
@kaizencc
Copy link
Contributor

Now this PR relies on #18102. It's a little controversial; we may have to find some other workaround.

mergify bot pushed a commit that referenced this pull request Dec 21, 2021
…18102)

This PR introduces a proposed new label, `pr-linter/exempt-breaking-change` that, when added, circumvents the check that asserts stable modules do not have breaking changes. 

Motivation: A situation like #18027 where we have are willing to accept a functional breaking change to a stable module. The regular `allowed-breaking-changes.txt` file does not work here, since there is no breaking change to the API. We want to be able to document the breaking change, but by documenting we alert `prlint` that we are breaking a stable module.

Counterargument: Functional breaking changes were explicitly banned in #14861. From the PR description: "The CDK must be more strict on preventing such changes and the impact due to their perception."

I also added some "manual linting" to the file myself since it was bothering me, and now it muddies the diff. Sorry!

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@kaizencc kaizencc added pr-linter/exempt-breaking-change The PR linter will not require stability in stable modules and removed pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version. labels Dec 21, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 21, 2021

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: f1bc543
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit fd149b1 into aws:master Dec 21, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 21, 2021

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).

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…ws#18102)

This PR introduces a proposed new label, `pr-linter/exempt-breaking-change` that, when added, circumvents the check that asserts stable modules do not have breaking changes. 

Motivation: A situation like aws#18027 where we have are willing to accept a functional breaking change to a stable module. The regular `allowed-breaking-changes.txt` file does not work here, since there is no breaking change to the API. We want to be able to document the breaking change, but by documenting we alert `prlint` that we are breaking a stable module.

Counterargument: Functional breaking changes were explicitly banned in aws#14861. From the PR description: "The CDK must be more strict on preventing such changes and the impact due to their perception."

I also added some "manual linting" to the file myself since it was bothering me, and now it muddies the diff. Sorry!

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…ot an endpoint (aws#18027)

imported Domain's domainEndpoint should be a endpoint and not a url.
Fixes [aws#18017](aws#18017)

BREAKING CHANGE: imported domain property `domainEndpoint` used to contain `https://` prefix, now the prefix is dropped and it returns the same value as a `domainEndpoint` on a created domain

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-elasticsearch Related to Amazon Elasticsearch Service @aws-cdk/aws-opensearch Related to the @aws-cdk/aws-opensearchservice package pr-linter/exempt-breaking-change The PR linter will not require stability in stable modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-elasticsearch): imported Domain's domainEndpoint is the full URL, not the Endpoint
3 participants