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.

Sign up for GitHub

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

rfc 79: cdk v2.0 - change strategy on how deprecated APIs are handled #241

Merged
merged 4 commits into from
Sep 7, 2020
Merged

rfc 79: cdk v2.0 - change strategy on how deprecated APIs are handled #241

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fdfa051
rfc 79: cdk v2.0 - change strategy on how deprecated APIs are handled
Aug 24, 2020
af5386e
rewrite the section to be clearer
Aug 26, 2020
b23ce0f
some clarifications
Aug 26, 2020
29b3678
added note around deprecations from aws
Sep 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 74 additions & 19 deletions text/0079-cdk-2.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ v1 will be front-loaded. This includes all work that can be prepared without bre

## Forking v2 out of v1

The forking point will be achieved by cutting a new `next` branch from the current tip of the `master` branch, where the
The forking point will be achieved by cutting a new `v2` branch from the current tip of the `master` branch, where the
nija-at marked this conversation as resolved.
Show resolved Hide resolved
following changes will be made right away:

1. the repository-wide `version` configuration will be updated to `2.0.0-alpha` (the pre-release identifier could be
Expand Down Expand Up @@ -168,8 +168,8 @@ validated. Releases will be automatically published, however using the `next` NP
[github packages]: https://github.com/features/packages

As long as the codebases have not diverged too much, it should be possible to continue forward-porting new developments
on top of the `next` branch by simply cherry-picking new commits from the `master` branch. Alternatively, it might be
possible to simply periodically merge `master` into `next`. This task can easily be automated.
on top of the `v2` branch by simply cherry-picking new commits from the `master` branch. Alternatively, it might be
possible to simply periodically merge `master` into `v2`. This task can easily be automated.

A new standard operating procedure (SOP / MCM) will be made to support manually releasing a CDK `v2` release until this
process is fully automated. Additionally, canaries and integration tests need to be prepared to ensure the ongoing
Expand All @@ -184,9 +184,9 @@ _Thursdays_), manually resolving merge conflicts as needed.

The AWS Construct Library will be released as a single artifact, instead of a collection of more than 100 libraries. The
`monocdk-experiment` package has the tools necessary to re-package the existing _hypermodular_ libraries into a single
packaging. Re-using that same process will allow keeping the difference between `master` and `next` as constrained as
packaging. Re-using that same process will allow keeping the difference between `master` and `v2` as constrained as
possible (reducing the need for manual intervention in forward-porting features). No additional packaging work is needed
once the `next` branch has been forked from `master`, as the renaming of `monocdk-experiment` to `aws-cdk-lib` and
once the `v2` branch has been forked from `master`, as the renaming of `monocdk-experiment` to `aws-cdk-lib` and
marking all bundled packages `private` is taken care of at that time.

However, since users will all use a single `aws-cdk-lib` library, the granularity of information collected via the
Expand All @@ -210,15 +210,47 @@ The definition of `@deprecated` in the CDK specifies that annotated APIs will be
release. Consequently, we will systematically remove all `@deprecated` APIs from the codebase upon cutting a new major
version.

This task is expected to be easy to carry out by hand. Searching the codebase for `@deprecated` and deleting the
annotated code, as well as any private function that is no longer used. In the event non-`@deprecated` code is found to
depend on `@deprecated` code, the implementation in `master` will be fixed to no longer defer into the `@deprecated`
feature, and that fix will be forward-ported into the `next` branch in the exact same way that other changes are.
As described above in the [forking v2 out of v1](#forking-v2-out-of-v1) section, there will be a significant period of
time when both CDKv1 and CDKv2 will be in active development. During this time, they will be tracked in the source
code respectively by two active branches, `master` and `v2`, where changes on `master` will be forward merged onto `v2`.
Removing the deprecated APIs by hand right at the beginning is going to result in increased maintenance overhead caused
by merge conflicts. See [alternatives section](#removal-of-deprecated-apis---why-not-remove-these-by-hand) for more
details.

In order to prevent accidental introduction (or re-introduction) of `@deprecated` code in the `next` branch while
forward-porting new developments from `master`, a built-time check will be added that scans the codebase for
`@deprecated` (for example using `grep`), failing the build if occurrences are found. This is not done through
inspection of the _jsii assemblies_ as `@deprecated` code can exist outside of exported API signatures.
The following strategy will be used to reduce merge conflicts while shipping CDKv2 without deprecated APIs right from
its first release -

Before creating the `v2` branch,

- The `jsii` assembler will be updated to accept a `--no-deprecated` flag that will strip out all deprecated methods,
properties, classes and interfaces. This will be achieved via a typescript transform that is run before assembly
generation. A proof of concept for this transform is available [here](https://github.com/nija-at/cdk-deprecated-api-poc).

- `cdk-build` will be updated to recognize the environment variable `CDK_BLOCK_DEPRECATIONS` and set the
`--no-deprecated` flag on the `jsii` assembler.

- All current usage of deprecated APIs within the CDK repository will be removed. In addition, a new check will be
Copy link
Contributor

Choose a reason for hiding this comment

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

What about things that are deprecated because we think it's a bad idea to use them, but that we don't want to remove? (One example that springs to my mind are unversioned Engines in RDS, but I'm sure there's more of them)

We need some mechanism for preserving deprecated things that we don't want to remove in v2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's this new category of deprecated APIs?

If it's a bad idea for customers to use them, they should be removed, and customers should use the better APIs that should now be available.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's this new category of deprecated APIs?

If it's a bad idea for customers to use them, they should be removed, and customers should use the better APIs that should now be available.

That's not really an option, because L2s should model the entire service area of the service, even things we consider not to be good practices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still not clear what you mean.

In the example you've provided, they would use the versioned engines API, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but that would mean it's impossible to create an Instance/Cluster without a version, violating the above tenet.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the api will not be removed in the next major version is not deprecated. Maybe it's "NOT RECOMMENDED TO USE" but the term deprecated should not mean two different things. Keep it simple please

Copy link
Contributor Author

@nija-at nija-at Sep 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

+1 to @eladb.

It's not clear how much the IDE helps the user today anyway. In fact, we're now adding the word deprecated into the doc summary - aws/jsii@ce8c0c4

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, IDEs mark the usage of something deprecated pretty clearly (here's an example in Intellij).
Screen Shot 2020-09-04 at 8 51 12 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

If the api will not be removed in the next major version is not deprecated. Maybe it's "NOT RECOMMENDED TO USE" but the term deprecated should not mean two different things. Keep it simple please

This is not the definition of "deprecated" that I know. For example, there are APIs still available in Java that have been deprecated since the 90s.

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, IDEs mark the usage of something deprecated pretty clearly (here's an example in Intellij).

Screen Shot 2020-09-04 at 8 51 12 AM

That's not the case for VSCode unfortunately, which is probably the most used by our users.

For example, there are APIs still available in Java that have been deprecated since the 90s.

The fact that java decided to not remove these between major versions doesn't mean we should leave deprecated "debt" behind.

I think this is just about the semantics and I really prefer that keep the term "deprecated" reserved for APIs that will (most likely) be removed in the next major release (unless there are very unique circumstances that may require keeping it for another round). The api you are describing is not being deprecated but perhaps not recommended to use. If possible we should deprecate and remove it, if not, we should state in the docs and even add a warning if it's used.

added to pull requests on the `master` branch, that will run a full CDK build **and test** with the
`CDK_BLOCK_DEPRECATIONS` environment variable set. The typescript compilation and execution of the CDK's comprehensive
unit test suite should highlight any usage of deprecated APIs.

- In order to support unit tests that verify the behaviour of deprecated APIs, `cdk-test` will be updated to recognize
a new pragma `!/// !cdk-test:allow-deprecations` and the `CDK_BLOCK_DEPRECATIONS` environment variable. Tests holding
this pragma will be skipped when `CDK_BLOCK_DEPRECATIONS` is set.
nija-at marked this conversation as resolved.
Show resolved Hide resolved

For the first and every subsequent release of CDKv2,

- the release pipeline will be configured with the `CDK_BLOCK_DEPRECATIONS` environment variable that will run the
previously mentioned typescript transform as part of monolithic packaging (i.e., generation of `aws-cdk-lib`).

The `master` branch will remain in this state until we are ready to flip `v2` branch to be the new
`master`. At this point,

- a two day code freeze will be instituted and a one time clean up of all deprecated APIs from the source
code will be performed.
- The PR check will be removed from the `master` branch. Let the deprecations on CDKv2 begin!

[import rewrites]: https://github.com/aws/aws-cdk/tree/master/packages/%40monocdk-experiment/rewrite-imports

### Resetting v1 feature flags

Expand All @@ -241,8 +273,8 @@ retain the ability to disable as needed.
> | `@aws-cdk/core:enableStackNameDuplicates` | Update to new file names under `cdk.out` | 🚮 Remove |
> | `@aws-cdk/core:newStyleStackSynthesis` | Re-bootstrap environments | 🚮 Remove |

In order to reduce the codebase divergence between `master` and `next` (in order to minimize the amount of manual work
involved in forward-porting features on `next`), existing feature flags must be made version-aware, such that they are
In order to reduce the codebase divergence between `master` and `v2` (in order to minimize the amount of manual work
involved in forward-porting features on `v2`), existing feature flags must be made version-aware, such that they are
enabled by default (and impossible to disable if a clean and reasonably simple migration is possible) when the current
version is greater then `2.0.0-0`.

Expand Down Expand Up @@ -288,7 +320,7 @@ prerelease announcement should at least mention the following elements:

All breaking changes should be handled in the form of a deprecation in CDK v1. In the event a particular change cannot
be achieved in this way, a limited amount of time will be provided when _AWS Construct Library_ owners (as well as third
party contributors) will be allowed to introduce API-breaking changes in the `next` branch. These should be limited to
party contributors) will be allowed to introduce API-breaking changes in the `v2` branch. These should be limited to
items that were identified and documented as part of the [Prerelease Announcement](#prerelease-announcement). Additional
changes can however be introduced if a well documented proposal achieves consensus among the core development team. All
breaking changes (including those planned in the prerelease announcement) will be documented in the release notes for
Expand Down Expand Up @@ -332,9 +364,9 @@ Once the criteria have been satisfied, the high-level procedure for officially r
1. Create a new `v1` branch from the `master` branch
1. Configure GitHub branch protection on `v1` as appropriate
1. Deploy CI/CD infrastructure on top of `v1`
1. Make `next` the new `master` branch:
1. Force-push `next` on `master`
1. Delete `next` and all related CI/CD infrastructure
1. Make `v2` the new `master` branch:
1. Force-push `v2` on `master`
1. Delete `v2` and all related CI/CD infrastructure
1. Perform a _normal_ release
1. Ceremoniously annouce CDK v2 is _Generally Available_, and repeat the v1 maintenance plan

Expand Down Expand Up @@ -461,6 +493,29 @@ unless we stop supporting the `@aws-cdk/` packages.
Since we want to eventually supplant the current module structure with the new one, semver says we have to do it with
the release of a major version.

## Removal of @deprecated APIs - Why not remove these by hand?

An alternative that was considered is to remove these by hand in the `v2` branch. As of Aug 24, 2020, there are ~250
deprecated properties, methods and classes combined, distributed across ~80 files.

While most of these would be properties and simple public APIs being deprecated, they also include larger sections
such as entire classes. The main challenge with this approach are the merge conflicts that will arise when merging
changes from the `master` branch onto the `v2` branch.

Simple changes to files that contain a few deprecated APIs will pose no difficulty in `git` resolving these conflicts.
However, any changes to the implementation of a deprecated API, file refactor or any other major changes will require
non-trivial human intervention. It is unclear who owns the resolution of these conflicts, at what point they will be
resolved (i.e., during PR, during forward merge, during pipeline approval).

We expect the forked state of the system (i.e., two active branches) to exist for several months as we work through the
changes to have 'CDK 2.0' generally available. As we get closer to 'CDK v2.0', we must expect the list of major changes
to this major release to expand. This may manifest as major or significant changes to the code and its structure.
We must also expect an increased rate of deprecated APIs across the CDK as the reality of CDKv2 gets closer, with the
team looking to use this opportunity to implement major changes.

The continuous overhead of conflict resolution or a ban on large refactors or changes is an unreasonable expectation on
the team for such an extended period of time.

# Adoption Strategy

All users will perform the following to adopt CDK v2
Expand Down