rfc 328 - polyglot assert #330

nija-at · 2021-06-02T12:37:06Z

Tracking issue: polyglot assert library #328
Rendered version

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

text/0328-polyglot-assert.md

rix0rrr · 2021-06-02T13:03:04Z

text/0328-polyglot-assert.md

+template.
+
+```ts
+inspect.assertMatchTemplateBeta1({


The assert everywhere seems a little redundant? Especially if we rename the inspect variable assert. How about:

Suggested change

inspect.assertMatchTemplateBeta1({

assert.matchTemplateBeta1({

(And same for assert.hasResourceBeta1 etc)

Adding the assert prefix makes it clear that these methods will throw if the conditions are not met.

I've left space, for the future, to add a matchTemplate() that returns true/false. Maybe premature(?).

text/0328-polyglot-assert.md

eladb · 2021-06-02T13:01:08Z

text/0328-polyglot-assert.md

+  Properties: { Foo: 'Bar' },
+  DependsOn: [ 'Waldo', 'Fred' ],
+}, {
+  part: ResourcePartBeta1.COMPLETE_BETA1,


Again, no need to add _BETA1 everywhere. It's sufficient that the top level type has that prefix...

Let's say I wanted to change the behaviour of this enum value. With your suggestion I would need to create a new enum ResourcePartBeta2 and then create new methods with Beta2 (or whatever) suffix everywhere this enum is used.

More pragmatic to keep the suffix, no?

I am not sure I can think of a reason you'd want to change the value of the enum member. It's normally not even needed to specify a value.

We should be consistent about these suffixes. If the entire type is "Beta", I don't see any value in actually polluting all the members with that suffix.

To @nija-at point, if we only use betaX on the containing type (e.g class), what do we do when we want to deprecate an internal member? For example, assume the autoScalingGroup class is in preview phase, and we only want to change the behavior of formAutScalingGroupName method, do we need to deprecate the entire autoScalingGroup class for that?

export class AutoScalingGroupBeta { public static fromAutoScalingGroupName(scope: Construct, id: string, autoScalingGroupName: string): ..... } }

My gut feeling is that it will be less ideal from a user experience perspective.

there are a lot of nuance here, I think we need to flush out the preview specification so we can be consistent across the code base.

I would just deprecate AutoScalingGroupBeta, and introduce AutoScalingGroupBeta2 with this new method, while proxying all the deprecated methods from the deprecated type to the new type.

How will this work for users consuming fromAutoScalingGroupName? If you proxy it, you will be introducing a breaking change for users of AutoScalingGroupBeta

Good callout. Of course this specific method will not be proxied as-is in order to preserve the legacy behavior in the deprecated class.

Agree with Elad here. Having to call fromAutoScalingGroupName() AutoScalingGroupBeta1.fromAutoScalingGroupNameBeta1() on the off-chance we will want to deprecate it seems like the wrong tradeoff to me.

Also, let's assume for a second we do go down this route, and call it AutoScalingGroupBeta1.fromAutoScalingGroupNameBeta1(). What do we call the replacement method then? AutoScalingGroupBeta1.fromAutoScalingGroupNameBeta2()? That looks pretty bad...

I think we are over indexing on how things look. Our main objective is to make sure it is clear that an API is in preview. The second objective is to make is easy for users to use preview APIs, will introducing a whole new class will make it harder for users to upgrade? It can be, as the class usage is more prevalent than a single method in a user application (more references to the AutoScalingGroup class in the code base than to fromAutoScalingGroupName).

I think we need to make this decision in a dedicated PR, try out a few use cases and flush out the details using code samples.

text/0328-polyglot-assert.md

skinny85

My two cents. In general, looks really good.

text/0328-polyglot-assert.md

skinny85 · 2021-06-02T20:08:07Z

text/0328-polyglot-assert.md

+The import statement you will need to use is -
+
+```ts
+import { assertions } from 'aws-cdk-lib';


Really? We're adding a testing library to aws-cdk-lib? This seems like a mistake to me...

Is this decision final?

This RFC is about making this decision.

Why is it a mistake to add a testing library to aws-cdk-lib?

@skinny85 what's your arguments against making the assertion library part of the AWS CDK. It's a common practice to include assertion utilities in standard libraries (see node.js's 'assert' for example).

We want to standardize test assertions to be an intrinsic part of the AWS CDK so it can be used as a foundation for the ecosystem.

The important side effect is that it including it in the AWS CDK itself will allow us to use it from within the AWS CDK codebase, even if/when we aws-cdk-lib becomes a truly monolithic codebase (after we stop supporting v1).

Do we have any concerns around the size of the CDK package? If so, this could be an argument against including assertions in the main package, which would force users to deploy a testing library to a production environment, where it's not used. I know with Javascript we can use things like webpack, but I'm not sure whether this can be done in other languages. Then again, depending on the additional size, this may not even be relevant.

We do have concerns around the size but the assertion library is negligible comparing to the two huge lambda layers we bundle into the library, which is where we should optimize (see here).

text/0328-polyglot-assert.md

skinny85 · 2021-06-02T20:17:45Z

text/0328-polyglot-assert.md

+Given this and working backwards from the key requirement, the new assert module
+must be part of the AWS CDK monorepo.


This logic doesn't convince me.

Why can't this be a separate module that depends on aws-cdk-lib?

How would you satisfy the requirement I've identified (in the above section) if this is a separate module?
Do you disagree with that requirement?

We can ship a separate experimental assert library from a module in the monorepo, re-writing it as needed at build time.

Same way as we re-write the stable modules today to package them as part of monocdk, and the same way as we plan to ship the existing experimental modules like @aws-cdk/aws-appmesh (which has version 1.107.0) as new modules @aws-cdk-lib-experiments/aws-appmesh (with version 0.1.0).

I'll point you again towards the requirements section and quote it here, because your reply seems to ignore it -

The key requirement for the design choices is dogfooding.
The new assert APIs must be usable from within the AWS CDK, and over time,
all CDK modules that use the old assert module should be migrated to the new one.

If we ship this as an experimental module, I believe we cannot take declare a dependency on it from any of our stable modules.

If we ship this as an experimental module, I believe we cannot take declare a dependency on it from any of our stable modules.

Pretty sure we can, because it will be a devDependency.

Why deal with all this complexity? If we just publish this as any CDK construct library, it keeps things simple and gets the job done. We can deal with it the same way we handle everything else.
Why do you not like shipping this as any other module? The only difference I see is that these does not participate in synthesis, which is acceptable.

Because it bloats the aws-cdk-lib module with test code, and forces you to use the ugly Beta1 names.

I honestly don't see the problem here. We will have a module in the monorepo, @aws-cdk/assertions or something, which the single (v1) modules will depend on. This takes care of dogfooding. Then we re-package the module to be released with whatever name we want, @aws-cdk-lib/assertions or something else, in version 0.x.y. It will peerDepend on aws-cdk-lib.

This is exactly the process we want to have for other experimental modules.

Ok. Thanks for the feedback.

I'll take that into consideration before my final decision.

@skinny85 I am not sure I understand your arguments against including this is the standard library (besides module size, which is not a very strong argument given this is likely to add a few KiBs).

It also allows us to skip the ugly Beta1 suffixes.

That's a fair argument.
If we can release this library as an experimental module without adding any further complexity, we can do so.

However, after graduating to stable, the module will be included in aws-cdk-lib (just like any other experimental module).

text/0328-polyglot-assert.md

eladb · 2021-06-06T11:57:45Z

text/0328-polyglot-assert.md

+
+```py
+# In Python
+assertion.assertResourceCountIs('Foo::Bar', 2)


Suggested change

assertion.assertResourceCountIs('Foo::Bar', 2)

assertion.assert_resource_count_is('Foo::Bar', 2)

eladb · 2021-06-06T11:58:01Z

text/0328-polyglot-assert.md

+    "Qux": [ "Waldo", "Fred" ],
+  } """;
+
+assertion.assertHasResource('Foo::Bar', json.loads(expected))


eladb · 2021-06-06T11:59:11Z

text/0328-polyglot-assert.md

+  new AssertResourceOptionsBeta1.Builder().part(ResourcePartBeta1.COMPLETE).build());
+```
+
+```py


I have to say that I feel the multi language readme is a bit too much. I don't see why this RFC is any different than any README/RFC we have written so far in which we write the typescript story and have JSII map it to other languages.

Just makes it harder from a reader's perspective to focus on the important things, which are the assertions API.

This is not a JSII RFC...

Generally, I agree with you.

The part I want to emphasize in the RFC is that, for the best ergonomics in non-javascript languages, users will have to use a separate 3p library to convert to dict in Python and Map in Java, etc.
My examples show the use in Gson (for Java) and json (for Python).

Calling out that experience is important.

Because of the other languages, should we consider a string-based API as well? To avoid all of that low-level stuff with Gson / json?

// in Java assert.assertTemplateMatchesString("""{ "Resources": { "Type": "Foo::Bar", "Properties": { "Baz": false } } }""");

# in Python assertion.assert_template_matches_string("""{ "Resources": { "Type": "Foo::Bar", "Properties": { "Baz": false } } }""")

Thoughts?

Feels like we can add that later

Definitely a nice idea that we can add as the library evolves.

eladb · 2021-06-06T11:59:44Z

text/0328-polyglot-assert.md

+| Language              | Pkg Manager | Package                                       |
+| --------------------- | ------------|---------------------------------------------- |
+| typescript/javascript | npm         | `@aws-cdk/assertions`                         |
+| Java                  | Maven       | `software.amazon.awscdk.assertions`           |


why?

Isn't it a valid FAQ that users would expect to see? especially given that, the primary value prop of this RFC is that the module is available in all languages.

Of course it is but just feels like it's a detail we don't have to expand on in the RFC since it's derived from the submodule name and there isn't any control here.

eladb · 2021-06-06T12:02:18Z

text/0328-polyglot-assert.md

+The import statement you will need to use is -
+
+```ts
+import { assertions } from 'aws-cdk-lib';


@skinny85 what's your arguments against making the assertion library part of the AWS CDK. It's a common practice to include assertion utilities in standard libraries (see node.js's 'assert' for example).

We want to standardize test assertions to be an intrinsic part of the AWS CDK so it can be used as a foundation for the ecosystem.

The important side effect is that it including it in the AWS CDK itself will allow us to use it from within the AWS CDK codebase, even if/when we aws-cdk-lib becomes a truly monolithic codebase (after we stop supporting v1).

eladb · 2021-06-06T12:08:43Z

text/0328-polyglot-assert.md

+Given this and working backwards from the key requirement, the new assert module
+must be part of the AWS CDK monorepo.


@skinny85 I am not sure I understand your arguments against including this is the standard library (besides module size, which is not a very strong argument given this is likely to add a few KiBs).

eladb · 2021-06-06T12:53:56Z

LGTM approved as far as I am concerned

skinny85

LGTM.

Pull request has been modified.

rfc 328: polyglot assert

9351350

nija-at self-assigned this Jun 2, 2021

nija-at force-pushed the nija-at/polyglot-assert branch from afb91fa to aad9ed4 Compare June 2, 2021 12:38

update PR

f192c8b

nija-at force-pushed the nija-at/polyglot-assert branch 2 times, most recently from 2b886b7 to 171f267 Compare June 2, 2021 12:45

fix formatting

2a3c9e0

nija-at force-pushed the nija-at/polyglot-assert branch from 171f267 to 2a3c9e0 Compare June 2, 2021 12:45

add another section

f1ac1f7

rix0rrr reviewed Jun 2, 2021

View reviewed changes

eladb reviewed Jun 2, 2021

View reviewed changes

feedback

8512f87

nija-at requested a review from rix0rrr June 2, 2021 15:35

more changes

85cd9fd

skinny85 reviewed Jun 2, 2021

View reviewed changes

ericzbeard reviewed Jun 2, 2021

View reviewed changes

text/0328-polyglot-assert.md Outdated Show resolved Hide resolved

Niranjan Jayakar added 2 commits June 3, 2021 10:48

fluent apis and keep beta suffix contained

0aa8145

add java & python snippets

4ebf3bb

nija-at force-pushed the nija-at/polyglot-assert branch from 94dfb1d to 4ebf3bb Compare June 4, 2021 10:45

eladb reviewed Jun 6, 2021

View reviewed changes

Niranjan Jayakar added 2 commits June 9, 2021 10:07

fix langs

93d65ac

make the module experimental

6e872df

nija-at added the status/final-comment-period Pending final approval label Jun 9, 2021

skinny85 previously approved these changes Jun 9, 2021

View reviewed changes

py adjust

955e945

nija-at merged commit 3bd681d into master Jun 11, 2021

nija-at deleted the nija-at/polyglot-assert branch June 11, 2021 12:58

nija-at removed the status/final-comment-period Pending final approval label Jun 11, 2021

nija-at added the status/approved Ready for implementation label Jun 11, 2021

nija-at mentioned this pull request Aug 26, 2021

RFC 249: Rewrite the V2 Experiments RFC to have a more working-backwards tone #377

Merged

	inspect.assertMatchTemplateBeta1({
	assert.matchTemplateBeta1({

		Given this and working backwards from the key requirement, the new assert module
		must be part of the AWS CDK monorepo.

	assertion.assertResourceCountIs('Foo::Bar', 2)
	assertion.assert_resource_count_is('Foo::Bar', 2)

rfc 328 - polyglot assert #330

rfc 328 - polyglot assert #330

Conversation

nija-at commented Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NetaNir Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NetaNir Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NetaNir Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Jun 6, 2021

skinny85 left a comment

Choose a reason for hiding this comment

nija-at commented Jun 2, 2021 •

edited

Loading

NetaNir Jun 3, 2021 •

edited

Loading

NetaNir Jun 3, 2021 •

edited

Loading

NetaNir Jun 3, 2021 •

edited

Loading

skinny85 Jun 9, 2021 •

edited

Loading