chore(appconfig): fix some awslint errors, explicitly exempt others #28671
Conversation
github-actions
bot
added
valued-contributor
aws-cdk-automation
added
the
pr/needs-maintainer-review
paulhcsun
added
the
pr-linter/exempt-integ-test
| "props-physical-name:@aws-cdk/aws-appconfig-alpha.ApplicationProps", | ||
| "props-physical-name:@aws-cdk/aws-appconfig-alpha.DeploymentStrategyProps", | ||
| "props-physical-name:@aws-cdk/aws-appconfig-alpha.EnvironmentProps", | ||
| "props-physical-name:@aws-cdk/aws-appconfig-alpha.ExtensionProps", | ||
| "events-in-interface", |
There was a problem hiding this comment.
How is this awslint error fixed in this PR?
There was a problem hiding this comment.
Fixed by adding these methods to the application interface
There was a problem hiding this comment.
@chenjane-dev I've done my best to explain the specific linter errors below. There's a few points that require further discussion that I will follow up with later.
"no-unused-type:@aws-cdk/aws-appconfig-alpha.PredefinedDeploymentStrategyId" - This is for customers to use in case they want to import an AppConfig predefined deployment strategy
- the reason why this linter error flares up in this use case is because we are specifying an
enuminto a property that allows astring. I'm not sure if this even works for non-TS languages, but either way, this is a CDK anti-pattern. The correct path forward imo is to make aDeploymentStrategyIdenum-like class with a public constructor to specify arbitrary strings and the same static methods for thePredefinedIds. And make that the type fordeploymentStrategyId, which is a breaking change.
"props-physical-name:@aws-cdk/aws-appconfig-alpha.ApplicationProps"
"props-physical-name:@aws-cdk/aws-appconfig-alpha.DeploymentStrategyProps"
"props-physical-name:@aws-cdk/aws-appconfig-alpha.EnvironmentProps"
"props-physical-name:@aws-cdk/aws-appconfig-alpha.ExtensionProps" - These names are not prefixed by [cfnResource]. This is to help distinguish that these parameters are not set at deployment time.
- i think you misunderstand this one. The ask here is to standardize the naming of the resources. Instead of
nameunderApplicationProps, it should beapplicationName. This is to standardize with our other resources i.e. lambda function name isfunctionName. Remediation here is to make breaking changes to allnameprops with[Resource]Name.
"events-method-signature" - These on methods are not associated with CloudWatch Events
The reason for the linter rule is to reserve onXxx APIs for CloudWatch Events. This provides a CDK pattern where any API that begins with on is an events API. Therefore, your justification does not really move the needle for me. In this case, I see that the phrasing comes from appConfig itself. I will double check internally. If we do accept the exemption, I would still want you to specify each individual linter exemption rather than turning off the rule entirely (I didn't have time to do this when I was turning on awslint). Feel free to add that in to this PR.
"events-generic" - There already is a generic method. This is named on instead of onEvent for clearer naming since the action point will be passed in
The issue is the same as above -- are we ok with onXxx methods not being CW event methods. Is there any part of the original RFC that discusses this? And again, if we go through with the exemptions it will have to be each individual exemption.
- Finally, though not a blocker to stabilization, all the missing docs present a poor experience for users and should be updated where appropriate.
The first two fixes should come in separate PRs and call out their breaking changes.
aws-cdk-automation
removed
the
pr/needs-maintainer-review
|
Will follow up with the first two in separate PRs. Added the bottom two individual exceptions. Yes, this was designed and approved through the RFC: https://github.com/aws/aws-cdk-rfcs/blob/main/text/0499-appconfig-constructs.md#example-4 |
mergify
bot
dismissed
kaizencc’s stale review
Pull request has been modified.
|
Thanks for the quick link @chenjane-dev . I will double check with @otaviomacedo (bar raiser, i believe) and @rix0rrr (linter rule creator) for a final decision |
aws-cdk-automation
added
the
pr/needs-maintainer-review
| "events-method-signature", | ||
| "events-generic" | ||
| "events-method-signature:@aws-cdk/aws-appconfig-alpha.Application.on", | ||
| "events-method-signature:@aws-cdk/aws-appconfig-alpha.Application.onDeploymentBaking", |
There was a problem hiding this comment.
@chenjane-dev nit: i know i told you to add each individual linter exemption, because if not, and we stabilized, then we would be exempting events-method-signature everywhere. However, I don't think we need the individual params[0] and params[1] either. Having the onDeploymentBaking linter error is enough uniqueness i think
There was a problem hiding this comment.
I didn't include them at first but then was getting the errors on the params[0] and params[1]
There was a problem hiding this comment.
ok i'll check this out for you. seems weird if true but i believe you :)
kaizencc
changed the title
Linter rules caught that CDK standardizes resource name prop as `[resource]Name`. Previously this module only used `name` for the prop. Follow up from #28671. BREAKING CHANGE: `ApplicationProps.name` renamed to `ApplicationProps.applicationName` - **appconfig**: `EnvironmentProps.name` renamed to `EnvironmentProps.environmentName` - **appconfig**: `DeploymentStrategyProps.name` renamed to `DeploymentStrategyProps.deploymentStrategyName` - **appconfig**: `ExtensionProps.name` renamed to `ExtensionProps.extensionName` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
aws-cdk-automation
removed
the
pr/needs-maintainer-review
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…her than a string (#28743) Previously, we were typing this as a `string` and providing an enum for `PredefinedDeploymentStrategyId`s. This is a CDK anti-pattern because this makes the enum undiscoverable, since users see that it is typed only as a `string`. It also may not work in non-TS languages. Instead, we are moving the type to explicitly be an enum-like class. Follow up from #28671. BREAKING CHANGE: `deploymentStrategyId` prop in `fromDeploymentStrategyId` now takes a `DeploymentStrategyId` rather than a `string`. To import a predefined deployment strategy id, use `DeploymentStrategyId.CANARY_10_PERCENT_20_MINUTES`. Otherwise, use `DeploymentStrategyId.fromString('abc123')`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Auditing awslint errors.
Reasons for keeping current list of exemptions:
"props-physical-name:@aws-cdk/aws-appconfig-alpha.DeploymentStrategyProps"
"props-physical-name:@aws-cdk/aws-appconfig-alpha.EnvironmentProps"
"props-physical-name:@aws-cdk/aws-appconfig-alpha.ExtensionProps" - will be fixed in a separate PR.
Closes #27894 alongside #28742 and #28743
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license