-
Notifications
You must be signed in to change notification settings - Fork 89
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
RFC 249: V2 Experiments #250
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments.
In general, I feel like I'm missing some details. What are the names of the experimental modules? What are their versions? Are they released together, or does each and every one have a separate version? How does this affect our usage statistics gathering?
In my opinion, releasing them as separate modules is a mistake. This will bring back the same dependency problems that mono-CDK was designed to solve, except just for the experimental modules. What this will effectively mean is that 1) experimental modules cannot depend on other experimental modules, and 2) third-party modules cannot depend on experimental modules. This will make experimental modules much less useful than today, which will affect their usage (I suspect it will be considerably lower than today), which will make graduating them more difficult (less customer feedback and engagement). I consider that a serious risk.
I also don't see why do we need a separate repository for this. Why can't we release from the one we have today, just separately for the experimental modules? This hasn't been addressed in the doc at all.
@skinny85 experimental modules will be just like any other third party construct, any limitation they will have will also effect the eco system, which is what worries me in your statement. Maybe we need to think about a solution to that (huge) problem. |
I don't think that statement is true though. The Construct Library is special in the ecosystem - if it was the same as other third-party modules, we wouldn't be doing mono-CDK in the first place. |
I pulled some data from our dependencies. Here's all instances of stable modules depending on unstable ones:
(this is after I've marked None of those dependencies would be possible after this change. |
Another serious dependency issue is that we have a lot of |
And here is the list of unstable modules depending on each other - relevant to the discussion whether there should be a single experimental package released, or multiple ones:
|
Why is that true? What's special about the L2s? We are doing monocdk because of the inevitable dependencies between L2s which stems from the dependencies between AWS services (i.e you need S3 bucket for triggering a lambda etc.). Every L2 will depend on aws-cdk/aws-s3, aws-cdk/aws-iam, aws-cdk/aws-cloudwatch... for the most basic construct you will need to depend on at least 30 aws-cdk/* modules, this is why for these modules monocdk make sense. On the other hand you will have modules like aws-cdk/aws-synthetics that are more like third party constructs in that aspect - much less construct will depend on them. Are we saying that third party constructs can't depend on each others as well? What is the technical difference between aws-cdk/aws-synthetics and any other third party construct? |
So is your argument that |
I don't know if it should be outside of monocdk forever, overall it is nice that everything is in the same package. I was giving it as an example to an L2 which is similar to a third party construct in terms of dependencies. And it's not my argument, I was pointing out the fact that experimental modules, if released separately, will be the same as third party packages, if we think it's impossible to maintain/use then we should solve it, as it will hurt the eco system. |
Is it even feasible to go with the multi-repository approach here at all? Let's say a customer wants to submit a PR adding a new L2 to a module that was previously CFN-only. They can't do that, because there is no experimental module for this AWS service, and thus no GitHub repository for it, so there is nowhere to submit the Pull Request... |
Yeah you are right. But how does this connect with the Uluru import solution? Eventually we are working towards a point where L1s are going to be generated on-demand by users same as in So we won't need these empty L1 modules anyway... we will just have a projen project type for AWS CDK modules that will generate a module repo and people can submit that as their PR. |
Re: the discussion on separate vs one module, and what should go into mono-CDK. I want to summarize the existing points. We can do one of two things.
I'm personally leaning towards 1., but I don't care that much. What I really don't want is the current proposal in this RFC, which essentially buckets the modules as "special" on the criteria that "if it has an L2 contributed to it before the 2.0 release, then it's automatically "special" (even if it's a brand new L2 that was pushed a week before), and every module that doesn't have an L2 before that date is not special". |
I'm a little confused by the discussion around this point, so let's use a concrete example. If a member of the community wants to submit an L2 for, let's say Neptune, after we go to v2, what do they do? If we go with one repo per experimental module, they would need to ask us to first create the repo. If we go with a single repo for all experiments, then they could submit a PR with the new code. It's logistically more work for us to create a repo per module, but maybe it's a good thing to force a discussion about the new construct library before accepting a new PR out of nowhere. Do we want contributors to do the initial work in their own personal repositories first? What would the process be to convert an L2 that someone wrote into an "official" AWS experimental L2? Adam makes good points for not creating any new repositories at all, and using the build and package process to make sure we never introduce any breaking changes to a 2.x minor release, but I worry that it will be difficult to enforce. |
I don't think it's a matter of which is more work.... I don't think the "Git repository per package" is viable at all - where would the customer "ask us to first create the repo"...? I don't think we have a communication channel with customers that can be reliably used for that...
I feel like that will actually be more work for us, not less. If they submit PRs with us starting from the beginning, we can make sure they're small, and they get feedback from us early in the process. If we have to move an entire, already existing L2 into our repository, that will be a huge PR (that might involve dramatic changes to the library, that we didn't have the opportunity to specify earlier). In fact, we already have few of those (SageMaker?) that we refused to do, exactly for that reason.
We do this already for stable modules in 1.x, so this is a non-issue. |
Or |
God, please no. Git submodules are terrible. I would avoid them like the plague. |
text/0249-v2-experiments.md
Outdated
This RFC proposes that we stop experimenting in the main repository. To keep the implementation simple and to avoid | ||
possible inadvertent breakages, especially considering the need to convert from Typescript into a variety of languages, | ||
we should not use any clever techniques to simply hide the experiments. The code should be all stable. A user that | ||
installs aws-cdk-lib using npm or pip or any other package manager should be confident that there will be no intentional | ||
breaking changes in the 2.x line of releases for its lifetime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph can be made crisper by dropping the statements around "clever techniques to simply hide the experiments".
We're in the 'Motivation' section, so this feels off place. Perhaps this can be cleaned up and moved as the second paragraph under 'Summary'.
Pull request has been modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments from me. I see some naming inconsistencies that I think we need to sort out, and some details look left over from the previous versions.
General note: I would suggest using Markdown code backtics to enclose names used in code (so, aws-cdk-lib
, not aws-cdk-lib). Makes it much easier to read that way, as the names stand out much more.
I think Elad's suggestion ("What if experimental packages are just in a sub-namespace of the main package?") is a great one, and worth exploring. It would allow us to keep the capability of stable modules to depend on experimental ones, which the current proposal bans.
Pull request has been modified.
Co-authored-by: Adam Ruka <[email protected]>
Co-authored-by: Adam Ruka <[email protected]>
…into v2-experimental
Co-authored-by: Niranjan Jayakar <[email protected]>
…into v2-experimental
Proposal for filling out "Implementation Plan", points 3, 4 & 5.
…into v2-experimental
Re-structure the document into 3 alternatives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Missing final recommendation
- How do we enable the goal "The CDK team can still perform API experiments" for APIs in stable modules?
|
||
When CDK version `2.0` is released to General Availability (GA), the main | ||
Construct Library package that we vend will no longer contain unstable code. | ||
Unstable modules, including modules in Developer Preview, will be vended |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe developer preview means it moves to the core module?
For those reasons, we consider it essential for the CDK team to retain the | ||
capability to perform experiments with our APIs (of course, only those that are | ||
clearly marked as such). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to add another goal which is to enable a healthy 3rd-party library ecosystem that "peer depends" on aws-cdk-lib (this is one of the main reasons we eventually decided that we can't introduce breaking changes to aws-cdk-lib).
(so, modules that are marked "stable" will no longer have the capability to have | ||
unstable APIs inside of them). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All modules are marked stable in aws-cdk-lib, no?
(so, modules that are marked "stable" will no longer have the capability to have | ||
unstable APIs inside of them). | ||
|
||
We will need to add validations to our build scripts that make sure this rule is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have these compatibility checks in our builds. We just need to disallow @experimental.
We will need to add validations to our build scripts that make sure this rule is | ||
never broken. | ||
|
||
### Why can't we vend unstable code in mono-CDK stable modules? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to appendix
decision, which I believe cannot be changed without bumping the major version of | ||
CDK. | ||
|
||
### Option 1: sub-package of `aws-cdk-lib` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option conflicts with "No unstable code in the main mono-CDK modules". I don't see a reason to propose it.
|
||
- Might be considered less explicit, as a customer never says they want to | ||
depend on a package containing unstable APIs, or with `0.x` for the version. | ||
- If a third-party package depends on an unstable API in a non-obvious way (for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes this a no starter
- It will not be possible to add new dependencies on unstable modules to | ||
stable modules in the future (for example, that's a common need for | ||
StepFunction Tasks). | ||
- Graduating a module to stable will be a breaking change for customers. We can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Graduating a module to stable will be a breaking change for customers. We can | |
- Graduating a module to stable will be a breaking change for customers who consume the unstable module from the experimental library. We can |
|
||
In this option, we will fork the CDK codebase and maintain 2 long-lived | ||
branches: one for version `2.x`, which will be all stable, and one for version | ||
`3.x`, which will be all unstable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using "3.x" is a bit misleading because this is not necessarily staging the next major version but rather just the experiments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about [email protected]
? I think that's the semver convention for pre-releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also a bit "wrong" because this is not the alpha for 3.x, it's a staging module for experimental APIs for 2.x, no?
|
||
### Require a feature flag for unstable code | ||
|
||
In this variant, we would add a runtime check into all unstable APIs that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we say there are no unstable APIs in aws-cdk-lib? what am I missing?
mitigate this downside by keeping the old unstable module around, but that | ||
leads to duplicated classes. | ||
|
||
### Option 3: separate multiple unstable packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this option. The constraints (disadvantages) feel reasonable.
We can mitigate this downside by keeping the old unstable package around, but that leads to duplicated classes.
Re: duplicate classes - we can completely delete the code off the code base and mark the released packages as deprecated, so no code duplication. Existing customers can continue to use them but won't get newer features.
### Option 4: separate V3 that's all unstable | ||
|
||
In this option, we will fork the CDK codebase and maintain 2 long-lived | ||
branches: one for version `2.x`, which will be all stable, and one for version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of this option. Maintaining 2 long-lived branches is not easy and is going to cause us a lot of maintainance pain. Separate repos or folders are much easier to manage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus, the disadvantages section points the other problems quite well.
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license