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(core-stage): Fix stage name check to be aligned with stack name #23089

Closed
wants to merge 1 commit into from
Closed

fix(core-stage): Fix stage name check to be aligned with stack name #23089

wants to merge 1 commit into from

Conversation

yuyokk
Copy link
Contributor

@yuyokk yuyokk commented Nov 25, 2022

Hi folks,

I faced the following errors today:

  • error 1
invalid stage name "Stage acme.example.com". Stage name must start with a letter and contain only alphanumeric characters, hypens ('-'), underscores ('_') and periods ('.')
  • error 2 (after fixing error 1)
Stack name must match the regular expression: /^[A-Za-z][A-Za-z0-9-]*$/, got 'DeploymentStage_acme.example.com-TenantCore'

I believe there is discrepancy in stage and stack names checks.

Please check changes on the PR and let me know if they make sense.
Thank you


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 Nov 25, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team November 25, 2022 15:11
@github-actions github-actions bot added p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK labels Nov 25, 2022
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.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mrgrain
Copy link
Contributor

mrgrain commented Nov 25, 2022

Hi @yuyokk I think this makes sense, but am very concerned about the implications of introducing more restrictive checks.

Could you please provide a reproduction example of how you run into this error, ideally by creating a new bug report or by linking an existing issue.

Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

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

Reproduction example required.

@yuyokk
Copy link
Contributor Author

yuyokk commented Nov 26, 2022

@mrgrain here is the bug #23098

@peterwoodworth
Copy link
Contributor

@mrgrain the reproduction posted looks good to me. Since stack names are built in part from stage names, would there be any case in which a stage is defined with . or _ and wouldn't contain a stack with these characters in the stack name? I don't think so, but maybe there's some edge case or functionality that isn't being accounted for here.

Essentially, for this to not be a breaking change that means that EVERY stage defined in any application also necessarily contains at least one stack whose name contains the stage name.

@mrgrain
Copy link
Contributor

mrgrain commented Nov 29, 2022

Thanks @peterwoodworth I agree with that assessment. I just haven't had time to look into the code path to confirm yet.

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

@mrgrain mrgrain self-assigned this Dec 17, 2022
@mrgrain
Copy link
Contributor

mrgrain commented Dec 19, 2022

Okay @peterwoodworth and @yuyokk This will be a breaking change and I'm really not sure if it's a good idea to have this fixed at all.

More discussions on the issue please, but here's what makes this breaking. The following code works right now but won't with the suggested change.

// somewhere in the pipeline stack
pipeline.addStage(new AppStage(this, "Stage_per_acme.example.com "));

// This is the Stage, note the provided stack name
class AppStage extends Stage {
  constructor(scope: Construct, id: string, props?: StageProps) {
    super(scope, id, props);

    new HelloCdkStack(this, "HelloCdkStack", {
      stackName: "hello"
    });
  }
}

@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 Dec 25, 2022
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

@yuyokk yuyokk deleted the fix-stage-name-check branch February 10, 2023 22:16
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 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants