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

chore(codepipeline-actions): change the name of the ServiceCatalogDeployAction #13780

Merged

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Mar 24, 2021

The ServiceCatalogDeployAction was an experimental Action in the codepipeline-actions module.
Stabilize it by renaming it ServiceCatalogDeployActionBeta1.
Also, remove the unused dependency between this module,
and service catalog module.

BREAKING CHANGE: the Action ServiceCatalogDeployAction has been renamed to ServiceCatalogDeployActionBeta1

  • codepipeline-actions: the type ServiceCatalogDeployActionProps has been renamed to ServiceCatalogDeployActionBeta1Props

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

@skinny85 skinny85 requested a review from NetaNir March 24, 2021 23:31
@gitpod-io
Copy link

gitpod-io bot commented Mar 24, 2021

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 24, 2021
Copy link
Contributor

@NetaNir NetaNir left a comment

Choose a reason for hiding this comment

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

@skinny85 thank you so much for leading the way on this! I was thinking that real life example will be the best way to decide on a naming scheme.

(summarizing the change in this PR for the sake of discussion)

There is one preview API introduced in this PR, compose of two parts, the method ServiceCatalogDeployActionPreview and it's props that are also named as Preview: ServiceCatalogDeployActionPreviewProps.

In the original RFC we discussed a few requirements for the naming scheme of Preview APIs:

some relevant discussion here

  1. Should include a version indicator, e.g xxx1. This is required so we can introduce additional preview versions before the final version.
  2. Should reflect the experimental nature of the API. The motivation being that while we are not going to introduce a non backward compatible changes, these API are not fully baked, might include bugs and will be deprecated (if only to introduce an identical API under the final name).
  3. Minimal "visual noise" (term stolen from @rix0rrr).

Adding all the above and applying it on the ServiceCatalogDeployAction API, I can think of a few options, please do add alternatives if any comes to mind:

  1. ServiceCatalogDeployActionBeta1 - beta is a good candidate as it is a widely used term and will resonate with users as a "non final version of the API". It does however misses on the "visual noise" aspect.
  2. ServiceCatalogDeployActionX1 - I was on the fence for this one when it was initially suggested, mainly because I thought it will not convey the idea behind the preview APIs concept. But I can see "The AWS CDK X APIs" section in our docs, and it also sound nice, e.g "What are the X APIs?". I think this can be less cumbersome than other options both from a coding perspective and "verbal simplicity" (easier to refer to, I can't think of a better way to phrase this.).
  3. ServiceCatalogDeployActionPre1 - Not sure this one actually adds value over beta. On one hand, Pre is for Preview, so it has the advantage of consistency, on the other hand, Pre does not have the same "familiar concept" advantage of beta . Adding it here in case someone really loves it.

I think Im leaning towards 2, what do you think?

@@ -90,7 +90,6 @@
"@aws-cdk/aws-iam": "0.0.0",
"@aws-cdk/aws-lambda": "0.0.0",
"@aws-cdk/aws-s3": "0.0.0",
"@aws-cdk/aws-servicecatalog": "0.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

@skinny85 skinny85 force-pushed the fix/service-catalog-deploy-codepipeline-action branch 2 times, most recently from 1b3514f to 27e2cbd Compare April 7, 2021 20:17
@skinny85
Copy link
Contributor Author

skinny85 commented Apr 7, 2021

In the original RFC we discussed a few requirements for the naming scheme of Preview APIs:

some relevant discussion here

  1. Should include a version indicator, e.g xxx1. This is required so we can introduce additional preview versions before the final version.
  2. Should reflect the experimental nature of the API. The motivation being that while we are not going to introduce a non backward compatible changes, these API are not fully baked, might include bugs and will be deprecated (if only to introduce an identical API under the final name).
  3. Minimal "visual noise" (term stolen from @rix0rrr).

Adding all the above and applying it on the ServiceCatalogDeployAction API, I can think of a few options, please do add alternatives if any comes to mind:

  1. ServiceCatalogDeployActionBeta1 - beta is a good candidate as it is a widely used term and will resonate with users as a "non final version of the API". It does however misses on the "visual noise" aspect.
  2. ServiceCatalogDeployActionX1 - I was on the fence for this one when it was initially suggested, mainly because I thought it will not convey the idea behind the preview APIs concept. But I can see "The AWS CDK X APIs" section in our docs, and it also sound nice, e.g "What are the X APIs?". I think this can be less cumbersome than other options both from a coding perspective and "verbal simplicity" (easier to refer to, I can't think of a better way to phrase this.).
  3. ServiceCatalogDeployActionPre1 - Not sure this one actually adds value over beta. On one hand, Pre is for Preview, so it has the advantage of consistency, on the other hand, Pre does not have the same "familiar concept" advantage of beta . Adding it here in case someone really loves it.

I think Im leaning towards 2, what do you think?

Thanks for the review @NetaNir. I went with option nr 1. I think ServiceCatalogDeployActionX1 looks weird, and "Beta" is not much more "noise", especially verbally, compared to "X", to justify this more cryptic way of naming.

Submitted a new revision, would appreciate another look!

@iliapolo
Copy link
Contributor

iliapolo commented Apr 9, 2021

I agree that option 3 doesn't really give any additional value.

As far as the other two, while I do like the sound of CDK X API's, I think it's less clear and isn't much of a visual noise reduction over beta. So my vote goes for beta as well.

@NetaNir
Copy link
Contributor

NetaNir commented Apr 9, 2021

I agree with the advantage of clarity for xxxBeta1. But, let me try and pitch the X suffix again: I think it sound better, from a "marketing" perspective. It's easier to refer to: "The X APIs". Its disadvantage of not being an industry wide term can also be viewed as an advantage as it means that there will be no assumption made on what does it mean in the context of the AWS CDK, we get to define it.

@skinny85
Copy link
Contributor Author

I agree with the advantage of clarity for xxxBeta1. But, let me try and pitch the X suffix again: I think it sound better, from a "marketing" perspective. It's easier to refer to: "The X APIs". Its disadvantage of not being an industry wide term can also be viewed as an advantage as it means that there will be no assumption made on what does it mean in the context of the AWS CDK, we get to define it.

I actually think "X APIs" sound weird, like we're doing R-rated APIs in CDK 🙂. So I don't see their advantage from a marketing perspective.

Talking about "Beta APIs" seems very natural to me in comparison.

@NetaNir
Copy link
Contributor

NetaNir commented Apr 15, 2021

I can't argue with that. beta it is

@mergify
Copy link
Contributor

mergify bot commented Apr 15, 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).

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Apr 15, 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).

…loyAction

The `ServiceCatalogDeployAction` was an experimental Action in the codepipeline-actions module.
Stabilize it by renaming it `ServiceCatalogDeployActionPreview`.
Also, remove the unused dependency between this module,
and service catalog module.

BREAKING CHANGE: the Action `ServiceCatalogDeployAction` has been renamed to `ServiceCatalogDeployActionPreview`
* **codepipeline-actions**: the type `ServiceCatalogDeployActionProps` has been renamed to `ServiceCatalogDeployActionPreviewProps`
@skinny85 skinny85 force-pushed the fix/service-catalog-deploy-codepipeline-action branch from 5a5cdec to 9ffa948 Compare April 15, 2021 21:30
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Apr 15, 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).

@mergify mergify bot merged commit a99e901 into aws:master Apr 15, 2021
@skinny85 skinny85 deleted the fix/service-catalog-deploy-codepipeline-action branch April 15, 2021 21:59
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
…loyAction (aws#13780)

The `ServiceCatalogDeployAction` was an experimental Action in the codepipeline-actions module.
Stabilize it by renaming it `ServiceCatalogDeployActionBeta1`.
Also, remove the unused dependency between this module,
and service catalog module.

BREAKING CHANGE: the Action `ServiceCatalogDeployAction` has been renamed to `ServiceCatalogDeployActionBeta1`
* **codepipeline-actions**: the type `ServiceCatalogDeployActionProps` has been renamed to `ServiceCatalogDeployActionBeta1Props`


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
njlynch added a commit that referenced this pull request Sep 30, 2021
With the commitment to not support breaking changes in stable modules (and all
of aws-cdk-lib), we need to document the standard for introducing APIs we know
to be potentially experimental or unstable.

This standard was originally proposed in https://github.com/aws/aws-cdk-rfcs/blob/master/text/0249-v2-experiments.md, and
was first actually used in #13780.

This task documents the standard in the CONTRIBUTING guide so we can use it as a
reference going forward.

closes #16434
mergify bot pushed a commit that referenced this pull request Oct 11, 2021
With the commitment to not support breaking changes in stable modules (and all
of aws-cdk-lib), we need to document the standard for introducing APIs we know
to be potentially experimental or unstable.

This standard was originally proposed in https://github.com/aws/aws-cdk-rfcs/blob/master/text/0249-v2-experiments.md, and
was first actually used in #13780.

This task documents the standard in the CONTRIBUTING guide so we can use it as a
reference going forward.

closes #16434


----

*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
With the commitment to not support breaking changes in stable modules (and all
of aws-cdk-lib), we need to document the standard for introducing APIs we know
to be potentially experimental or unstable.

This standard was originally proposed in https://github.com/aws/aws-cdk-rfcs/blob/master/text/0249-v2-experiments.md, and
was first actually used in aws#13780.

This task documents the standard in the CONTRIBUTING guide so we can use it as a
reference going forward.

closes aws#16434


----

*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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants