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(cloudfront): requirement of domainNames prevents moving a domain name between distributions #29329

Closed
wants to merge 3 commits into from

Conversation

Tietew
Copy link
Contributor

@Tietew Tietew commented Mar 1, 2024

Reason for this change

When I want to move a domain name from a distribution to another distribution,
I must create a distribution with a certificate associated but no domain names.

Description of changes

This PR removes the validation that domainNames must not be blank when a certificate is associated.

Description of how you validated changes

Updated a unit test to allow absent domainNames when a certificate is associated.

See AWS Documentation for details: Using custom URLs by adding alternate domain names (CNAMEs) > Moving an alternate domain name to a different distribution
https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/CNAMEs.html#alternate-domain-names-move

Checklist


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

@github-actions github-actions bot added the p2 label Mar 1, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team March 1, 2024 05:34
@github-actions github-actions bot added the valued-contributor [Pilot] contributed between 6-12 PRs to the CDK label Mar 1, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@Tietew
Copy link
Contributor Author

Tietew commented Mar 1, 2024

Exemption Request

There are no integ tests for a distribution with a certificate.

@aws-cdk-automation aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Mar 1, 2024
@mwebber
Copy link

mwebber commented Mar 13, 2024

@Tietew We are keen to see this change completed - is there anything we can do? It looks like it's waiting on a reviewer.

@msambol
Copy link
Contributor

msambol commented Mar 20, 2024

@Tietew Could you attach a screenshot of this behavior? Given there are no integ tests with certificates.

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Mar 31, 2024
@paulhcsun paulhcsun reopened this Apr 4, 2024
@paulhcsun paulhcsun added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Apr 4, 2024
@paulhcsun paulhcsun added pr-linter/do-not-close The PR linter will not close this PR while this label is present and removed closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. labels Apr 4, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review April 4, 2024 22:25

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@paulhcsun paulhcsun changed the title fix(cloudfront): requirement of domainNames prevents moving a domain name between distributions fix(cloudfront): requirement of domainNames prevents moving a domain name between distribution Apr 4, 2024
@paulhcsun paulhcsun changed the title fix(cloudfront): requirement of domainNames prevents moving a domain name between distribution fix(cloudfront): requirement of domainNames prevents moving a domain name between distributions Apr 4, 2024
Copy link
Contributor

@paulhcsun paulhcsun left a comment

Choose a reason for hiding this comment

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

Hey @Tietew, apologies for not getting around to this. I've reopened the PR and added the do-not-close label to it.

We require that every fix be associated to an issue so could you also create an issue for this and link it in the PR description body? Also as @msambol mentioned would you be able to attach some screenshots of the behaviour please?

@@ -79,6 +79,9 @@ be retrieved from `distribution.distributionDomainName`. CloudFront distribution
default. If you want to use your own domain name, such as `www.example.com`, you must associate a certificate with your distribution that contains
your domain name, and provide one (or more) domain names from the certificate for the distribution.

When you want to move a domain name between distributions, you can associate a certificate and specify no domain names.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When you want to move a domain name between distributions, you can associate a certificate and specify no domain names.
When you want to move a domain name between distributions, you can associate a certificate without specifying any domain names.

@@ -129,7 +129,7 @@ export interface DistributionProps {
*
* If you want to use your own domain name, such as www.example.com, instead of the cloudfront.net domain name,
* you can add an alternate domain name to your distribution. If you attach a certificate to the distribution,
* you must add (at least one of) the domain names of the certificate to this list.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is no longer something required, could you also add a sentence at the end explaining the case where we don't need to provide a domain name?

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 5dba751
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 4, 2024
@TheRealAmazonKendra TheRealAmazonKendra removed pr-linter/do-not-close The PR linter will not close this PR while this label is present pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Apr 17, 2024
@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label May 3, 2024
mergify bot pushed a commit that referenced this pull request Aug 21, 2024
…name between distributions (#31001)

### Issue # (if applicable)

Closes #29960.

### Reason for this change

When I want to move a domain name from a distribution to another distribution,
I must create a distribution with a certificate associated but no domain names.

### Description of changes

Re-submit of previous #29329.
Removed the validation that `domainNames` must not be blank when a certificate is associated.

### Description of how you validated changes

Updated a unit test to allow absent domainNames when a certificate is associated.

See AWS Documentation for details: Using custom URLs by adding alternate domain names (CNAMEs) > Moving an alternate domain name to a different distribution
https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/CNAMEs.html#alternate-domain-names-move

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
GavinZZ pushed a commit that referenced this pull request Aug 22, 2024
…name between distributions (#31001)

### Issue # (if applicable)

Closes #29960.

### Reason for this change

When I want to move a domain name from a distribution to another distribution,
I must create a distribution with a certificate associated but no domain names.

### Description of changes

Re-submit of previous #29329.
Removed the validation that `domainNames` must not be blank when a certificate is associated.

### Description of how you validated changes

Updated a unit test to allow absent domainNames when a certificate is associated.

See AWS Documentation for details: Using custom URLs by adding alternate domain names (CNAMEs) > Moving an alternate domain name to a different distribution
https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/CNAMEs.html#alternate-domain-names-move

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
GavinZZ pushed a commit that referenced this pull request Aug 23, 2024
…name between distributions (#31001)

### Issue # (if applicable)

Closes #29960.

### Reason for this change

When I want to move a domain name from a distribution to another distribution,
I must create a distribution with a certificate associated but no domain names.

### Description of changes

Re-submit of previous #29329.
Removed the validation that `domainNames` must not be blank when a certificate is associated.

### Description of how you validated changes

Updated a unit test to allow absent domainNames when a certificate is associated.

See AWS Documentation for details: Using custom URLs by adding alternate domain names (CNAMEs) > Moving an alternate domain name to a different distribution
https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/CNAMEs.html#alternate-domain-names-move

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*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
closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes valued-contributor [Pilot] contributed between 6-12 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants