feat(servicecatalog): ProductStackHistory can retain old ProductStack iterations #20244

wanjacki · 2022-05-06T17:41:02Z

Adding enhancement to ProductStack to allow the specification of a VersioningStrategy.
VersioningStrategy RetainPreviousVersions added to save previously deployed ProductStacks templates in a local context directory. These productVersions can then be easily be deployed using the stored templates.

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

gitpod-io · 2022-05-06T17:41:09Z

github-actions · 2022-05-10T00:09:10Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

wanjacki · 2022-05-10T15:46:58Z

Hi i am not sure how to fix the conflicting files. I tried both force updating and creating a separate commit but it does not seem to help. The second commit has no conflict with the first commit. It should be as simple as taking the latest packages/@aws-cdk/aws-servicecatalog/test/integ.product.expected.json so would the reviewer with write access be able to fix this?

github-actions · 2022-05-11T00:09:35Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

robertd · 2022-05-11T10:57:48Z

@wanjacki Probably due to #20210. Have you tried rebasing?

robertd · 2022-05-11T11:27:40Z

And port your changes from integ.product.expected.json to product.integ.snapshot/*.json as this is the new way of doing integ tests in CDK from now on.

…loyed ProductStacks.

robertd · 2022-05-11T18:51:49Z

@wanjacki Looks good. 👍

wanjacki · 2022-05-11T20:27:46Z

@robertd Yes got it working. Thanks alot

comcalvi

This is a good first attempt! Some clarification needs to be added around the versioningStrategy and how it works.

comcalvi · 2022-05-12T22:52:14Z

packages/@aws-cdk/aws-servicecatalog/README.md

+We can also define a `versioningStrategy` for your `ProductStack` deployment. 
+With the `Default` strategy, a new `ProductStack` is needed for each `productVersion`
+and each `productVersion` will get overwritten with the latest changes to your `ProductStack`.
+With the `RetainPreviousVersions` strategy, a local template will be retained for each `ProductStack` deployed.
+Subsequently, any `ProductStack` changes will not overwrite the current version.
+A new `productVersionName` must be specified in order for `ProductStack` changes to be deployed.
+When the `ProductStack` is updated with a new version, the previous version can still be deployed.
+The template of previously deployed `ProductStack` can be referenced using `fromProductStackContext`
+and passing the corresponding `ProductStack` id.


what happens if a user specifies the Default strategy, adds several new productVersions, then changes the strategy to RetainPreviousVersions? What happens in the inverse (specifying RetainPreviousVersions, and then switching to Default)? Are the retained versions then lost, if the strategy changes to Default?

This might be a sign of poor naming.

If a user specifies the Default strategy then switches to RetainPreviousVersions. Only versions deployed after the switch will be retained.
If they do the reverse, then all further deployed versions will no longer be retained. Previously retained versions will not be deleted.

Do you have a suggestion on how we can name this behavior better?

So RetainPreviousVersions and Default only apply to the version on which they are attached. I find this confusing because we have the concept of a Version referring to both a version of a product and a version of a version. I understand it's up to the user to attach semantics to these terms, but I'd prefer to see versions of a product called variants (or something better). @rix0rrr thoughts?

Are you not trying to say something like:

const product = new CloudFormationProduct(this, 'Product', { productVersions: [ { productVersionName: "v1", cloudFormationTemplate: CloudFormationTemplate.fromHistoricalSnapshot('v1'), }, ], });

?

The fact that it uses fromHistoricalSnapshot implies RETAIN_PREVIOUS_VERSIONS (because what else would you do).

comcalvi · 2022-05-12T22:59:07Z

packages/@aws-cdk/aws-servicecatalog/README.md

+    {
+      productVersionName: "v2",
+      cloudFormationTemplate: servicecatalog.CloudFormationTemplate.fromProductStack(new S3BucketProduct(this, 'S3BucketProduct')),
+      versioningStrategy: servicecatalog.VersioningStrategy.RETAIN_PREVIOUS_VERSIONS


this is a bit confusing. Why can the versioningStrategy be specified for each member of productVersions, instead of being set for the product? Setting v2 to have RETAIN_PREVIOUS_VERSIONS, and having v1 be DEFAULT, is confusing because RETAIN_PREVIOUS_VERSIONS implies that v1 will be retained, and I don't think that's the intent here. This would make more sense to me if the versioningStrategy was specified for the entire product, and not for each version.

versioningStrategy only applies to fromProductStack looks like... so shouldn't it be part of the fromProductStack constructor?

@rix0rrr is correct. The versioningStrategy currently only applies to ProductStacks.
The reason we cannot set it directly on the product is because we may have two separate productVersions for a single product.
For example a product may have:
v1_blue and v1_green, which can both be updated to a v2 version. We could have a strategy for the entire product, but that will enforce that strategy for each version. Making it tied to each version allows the user to have one use DEFAULT and another use RETAIN_PREVIOUS_VERSIONS

Setting a versioningStrategy on v1 would not do anything since it using fromProductStackContext. Think of the versioningStrategy as applying to that specific ProductStack rather than to a specific version.

The reason we choose not to make it apart of the fromProductStack constructor is because we want it leave it open to have future versioningStrategies that don't apply to just ProductStack. For example, we could add the RETAIN_PREVIOUS_VERSIONS or another strategy to fromAsset if there is a useful use case for it. However, I did originally have it in the constructor, but was suggested by Adam (left CDK team) to move it.
I can see how it is confusing, so if you think we should have it tied only to the fromProductStack constructor we can change this and we should still be able to make it more flexible in the future by adding an optional field to the other constructors.

I'm not sure I agree versioningStrategy applies only to fromProductStack(). A Product Version from an Asset (created using fromAsset()), or referencing some file uploaded somewhere (created using fromUrl()) can be changed in the same way one created from ProductStack can be, and so you might want to prevent those changes from being deployed the same way you want to prevent changes to ProductStack from being deployed.

Right we could extend the same functionality to fromAsset() or fromUrl(), however this PR itself is focusing on ProductStack since that was the issue a customer brought up with us.
With the other two the user can version their templates themselves either with additoinal local asset or from ie. git (and they pass in the additional urls).
The only way to version fromProductStack() is to create an entirely new ProductStack in code, which doesn't make sense the more versions we have and could result in a lot of duplicate code. We are trying to make it easier for them to do this by automatically saving their generated templates instead and making it easy to reference those templates.

Not sure I understand. If you store the URL, you would also pass the URL for the Version for that Product Version.

I mean when we try to read it from our local context. The user would only be passing in the productVersionName they would like to retrieve. We would need to determine if this productVersionName was stored from a productStack or from a URL (url would be stored in file instead of actual contents) as they would take different logic.

No one is asking us to retain Assets or retain URLs, and it seems like its will add more complexity, edge cases and possibly unintended behavior for the user.

I think its better to focus on the customer ask here and only retain Product Stacks. ChangeVersioningStrategy to RetentionStrategy as suggested by Adam and scope it down to a field in fromProductStack()

I mean when we try to read it from our local context. The user would only be passing in the productVersionName they would like to retrieve. We would need to determine if this productVersionName was stored from a productStack or from a URL (url would be stored in file instead of actual contents) as they would take different logic.

Of course, but that should be easy, right? We are the ones who control the format of these files, and the logic that both writes, and reads them. So I don't think there should be any issues with implementing this.

I think we are trying to design in too much flexibility here. All the customer was asking for is a way for them to update their ProductStack and make sure that their previously deployed copies of that ProductStack don't get overwritten.

Since this whole "retain versions" feature is not implemented in the Service, but layered on top by CDK, we're naturally going to be limited in what we can do. Whether assets should be able to be snapshotted can be debated, I definitely do not intend to be able to snapshot arbitrary URLs. If it was necessary to snapshot arbitrary URLs, then users can wrap a script that downloads to a file and proceeds to use assets, or something else.

But honestly, I think a new API makes more sense, rather than trying to shoehorn this behavior into the existing API with flags.

comcalvi · 2022-05-12T23:02:14Z

packages/@aws-cdk/aws-servicecatalog/lib/common.ts

+   * The strategy to use for a ProductStack deployment.
+   * Determines how a productVersion is saved and deployed.
+   */


minor style comment, but can these opening comments (/** and */) line up?

comcalvi · 2022-05-12T23:04:44Z

packages/@aws-cdk/aws-servicecatalog/lib/common.ts

+  /**
+   * Default Strategy for ProductStack deployment.
+   * This strategy will overwrite existing versions when deployed.


I don't understand these comments if a VersioningStrategy is defined for each version. This comment makes me think that this is defined per product; if one version has a retain strategy, and another version has the overwrite strategy, how do we decide which versions are retained? Are there versions of versions?

There can be versions of a versions.
It is more like one ProductStack is using a retain strategy, and another ProductStack has the overwrite strategy. Both ProductStacks are "versions" of the product.
I know its a bit confusing, but there currently is not implementation of versioning in Service Catalog and ProductStacks are a CDK-only concept. So we are attempting to kind of version ProductStacks in CDK code, but once its deployed to Service Catalog, each template is just a version of the Product.

I see. Effectively we have a product version A, B, C and then A, B and C can each have versions. I'd prefer we call product versions something other than versions, if we're also going to call the versions of versions "versions".

I think the mistake here is the VersioningStrategy name, as it re-uses the term "version" to mean something else than does it now.

How about calling it RetentionStrategy instead, with two values: OVERRIDE (the default), and RETAIN?

If we do this then let's call it OVERWRITE. But I favor a different type of API in general.

comcalvi · 2022-05-12T23:08:24Z

packages/@aws-cdk/aws-servicecatalog/lib/product.ts

+          }
+          break;
+        case TemplateType.PRODUCT_STACK_CONTEXT:
+          const templateFileKey = `${this.productPathUniqueId}.${template.productVersionDetails?.productStackId}.${productVersion.productVersionName}.product.template.json`;


what happens if template.productVersionDetails is undefined here?

It can't be undefined since we determined that that this is the case TemplateType is PRODUCT_STACK_CONTEXT.
In the constructor for CloudFormationProductStackContextTemplate we create the productVersionDetails, so it will always be defined.

class CloudFormationProductStackContextTemplate extends CloudFormationTemplate { private readonly productVersionDetails: ProductVersionDetails; /** * @param baseProductStackId The id of the product stack where the version was deployed from. */ constructor(public readonly baseProductStackId: string) { super(); this.productVersionDetails = new ProductVersionDetails(); this.productVersionDetails.productStackId = this.baseProductStackId; } }

The reason there's an optional check here is because productVersionDetails can be optional as in its not required for other TemplateType.

ahh okay, this makes sense. Thanks for clarifying. @rix0rrr should we use instanceof here instead? That would remove the ? operator here.

Never instanceof! If you see instanceof it's a bug.

@rix0rrr is that because inheritance should be used instead? Could you elaborate?

It's because instanceof for type testing of custom classes is unreliable. It's too big of a topic to go in here, but we can talk about it in person if you want.

comcalvi · 2022-05-12T23:09:37Z

packages/@aws-cdk/aws-servicecatalog/lib/product.ts

+      productVersions.push(
+        {


Suggested change

productVersions.push(

{

productVersions.push({

comcalvi · 2022-05-12T23:09:58Z

packages/@aws-cdk/aws-servicecatalog/lib/product.ts

+      );
+    }


Suggested change

);

}

});

ESLint is not happy and suggesting the original braces here.
Also the suggestion is moving the bracket before the parenthesis which doesn't make sense.

ahh woops that was the wrong pair

comcalvi · 2022-05-12T23:11:17Z

packages/@aws-cdk/aws-servicecatalog/test/product.test.ts

@@ -189,6 +191,66 @@ describe('Product', () => {
    expect(assembly.stacks[0].assets.length).toBe(1);
  }),

+  test('product test from product stack with versioning strategy retain', () => {


we need a test for the default case as well.

rix0rrr

This is a tough feature, and deviates A LOT from how CDK normally works.

Normally the service itself keeps product version histories (Lambda, CloudFormation) so CDK can just reason about the "current" state of the world and have the service keep track of changes.

I would urge you to reconsider implementing this in the service: not sure CDK is the place for it. If you want to push through, spend some more effort on naming and explaining the feature very well to users who haven't gone through the same design process you have and are intimately familiar with every idiosyncracity. Again: this is very different from how CDK usually behaves: be cognizant of that while explaining to users who are used to the usual way.

rix0rrr · 2022-05-13T08:00:55Z

packages/@aws-cdk/aws-servicecatalog/README.md

@@ -184,6 +184,48 @@ const product = new servicecatalog.CloudFormationProduct(this, 'Product', {
 });
 ```

+We can also define a `versioningStrategy` for your `ProductStack` deployment. 


Lead with the use case instead of the mechanism. WHY would a user want to read this section? Also explain how it works in general, because this has implications on the user's workflow, not just on the API they need to call. Add a section header.

I'd recommend something like:

## Immutable products The default behavior of Service Catalog is to overwrite each product version upon deployment. If instead you want to never overwrite existing versions, but only add new versions... (etc) ...all product templates will be written to disk, so that they will still be available in the future as the definition of the `ProductStack` subclass changes over time. **It is very important** that you commit these old versions to source control...

rix0rrr · 2022-05-13T08:05:24Z

packages/@aws-cdk/aws-servicecatalog/README.md

+We can also define a `versioningStrategy` for your `ProductStack` deployment. 
+With the `Default` strategy, a new `ProductStack` is needed for each `productVersion`
+and each `productVersion` will get overwritten with the latest changes to your `ProductStack`.
+With the `RetainPreviousVersions` strategy, a local template will be retained for each `ProductStack` deployed.
+Subsequently, any `ProductStack` changes will not overwrite the current version.
+A new `productVersionName` must be specified in order for `ProductStack` changes to be deployed.
+When the `ProductStack` is updated with a new version, the previous version can still be deployed.
+The template of previously deployed `ProductStack` can be referenced using `fromProductStackContext`
+and passing the corresponding `ProductStack` id.


This might be a sign of poor naming.

rix0rrr · 2022-05-13T08:09:51Z

packages/@aws-cdk/aws-servicecatalog/README.md

+With the `Default` strategy, a new `ProductStack` is needed for each `productVersion`
+and each `productVersion` will get overwritten with the latest changes to your `ProductStack`.
+With the `RetainPreviousVersions` strategy, a local template will be retained for each `ProductStack` deployed.
+Subsequently, any `ProductStack` changes will not overwrite the current version.


What does the workflow look while you are developing/testing the latest ProductStack?

Are you supposed to keep the most recent version at Default while iterating on it, and then flip it to RetainPreviousVersions as you are done?

From the code I'm reading, you must then remember to synth one final time and then commit.

And then, for the next version, change it to a fromTemplate, add a new fromProductStack with RetainPreviousVersions and repeat?

If that's what you are thinking, best to explain that here as well, because I'm not sure that's obvious to readers.

(As you are explaining the process to users, I hope you will then internally go "hmm that seems like a sharp edge that I can shave off by redesigning/guard against with validation")

The workflow would ideally have user stay with Default or stay with RetainPreviousVersions the whole way and not be constantly switching between the strategies.

If they are using RetainPreviousVersions. They would just update the version on their current fromProductStack ie. from v1tov2.
Then if they want the previous version to be deployed as well, they would have to add another version with fromProductStackContext

The end result would be the same as you explained, but I guess the workflow for the user would be to update their "current" version, then add any "previous" versions if they choose to, rather than change their "current" to use fromProductStackContext then add a new version.

I think you are making the assumption that a user gets their new version of the ProductStack exactly right on the first attempt. Remember that if we have RetainPreviousVersions enabled, what happens on synth is:

If no snapshot of the stack exists, we create it

If a snapshot already exists, the current stack definition must be exactly equal to the snapshot

That means:

If a user changes the product version name FIRST before changing the ProductStack and (accidentally or to check something) run cdk synth, their stack definition is now locked in. They can't change it anymore! (And they haven't even begun to make their changes yet)

If a user is using cdk watch, synthing is done continuously on their behalf. They may be synthing (and locking in their template) without even consciously taking an action.

Even if they made the changes to the ProductStack before changing the version, and synth it, and potentially even deploy it to their developer account... their changes are now ALSO locked in. Iteration is effectively impossible, hope they got it right on the first try!

That means that in a realistic development workflow, people will have to switch back and forth between those versioning strategies to iterate.

💡 Here's a tip that will last you the rest of your career: Blog post driven development. Look at the AWS Blog and read a couple of posts. Notice how they all take the customer by the hand and say: "to use this feature, you first do X, then Y, then Z", and they don't skip any steps or handwave. For every feature you design henceforth, try to do the same. Pretend you are writing an AWS Blog Post, write in a friendly voice to the customer that has a particular problem or use case, telling them how awesome your feature is and that they "only" need to do X, Y and Z in that order (and don't skip anything). This will force you to be explicit about certain things you are probably handwaving away right now, and potentially cause you to scratch your chin and internally go "oh that isn't as easy as I thought after all".

packages/@aws-cdk/aws-servicecatalog/README.md

rix0rrr · 2022-05-13T08:12:39Z

packages/@aws-cdk/aws-servicecatalog/README.md

+    {
+      productVersionName: "v2",
+      cloudFormationTemplate: servicecatalog.CloudFormationTemplate.fromProductStack(new S3BucketProduct(this, 'S3BucketProduct')),
+      versioningStrategy: servicecatalog.VersioningStrategy.RETAIN_PREVIOUS_VERSIONS


versioningStrategy only applies to fromProductStack looks like... so shouldn't it be part of the fromProductStack constructor?

rix0rrr · 2022-05-13T08:14:37Z

packages/@aws-cdk/aws-servicecatalog/README.md

+    },
+    {
+      productVersionName: "v1",
+      cloudFormationTemplate: servicecatalog.CloudFormationTemplate.fromProductStackContext('S3BucketProduct'),


Why is this called Context ? This is confusing because it's not like CDK context, plus context is a vague term.

Why not fromProductStackSnapshot ?

We can change this.

rix0rrr · 2022-05-13T08:25:00Z

packages/@aws-cdk/aws-servicecatalog/lib/common.ts

+/**
+ * Constant for the context directory to store retained ProductStack templates.
+ */
+export const PRODUCT_STACK_CONTEXT_DIRECTORY = 'product-stack-context';


Suggested change

export const PRODUCT_STACK_CONTEXT_DIRECTORY = 'product-stack-context';

export const PRODUCT_STACK_CONTEXT_DIRECTORY = 'product-stack-history';

?

If we are changing it to fromProductStackSnapshot. Shouldn't this be product-stack-snapshots as well?

wanjacki · 2022-05-13T14:59:17Z

"This is a tough feature, and deviates A LOT from how CDK normally works.

Normally the service itself keeps product version histories (Lambda, CloudFormation) so CDK can just reason about the "current" state of the world and have the service keep track of changes.

I would urge you to reconsider implementing this in the service: not sure CDK is the place for it. If you want to push through, spend some more effort on naming and explaining the feature very well to users who haven't gone through the same design process you have and are intimately familiar with every idiosyncracity. Again: this is very different from how CDK usually behaves: be cognizant of that while explaining to users who are used to the usual way."

@rix0rrr
In response to this, Service Catalog is actually trying to implement its own versioning currently, but it might not have any impact on this feature.
The reason why we can't move this specific feature to the service is because ProductStack only exist in CDK.
ProductStack essentially translate CDK code into a CFN template for Service Catalog to attach to a Product. This is why we can only really keep track of them in CDK. I will work on fixing the naming and explanation.

Sorry I should of included this design document I worked on with Adam Ruka that might help explain a bit more on the customer ask, motivations and some of the design choices we made:
q/rR8bACMbGZnb

The reason we decided to use fromProductStackContext is because we wanted each version to be explicitly defined in code which would make it work more like how CDK normally works. The originally design was much more anti-CDK in comparison, which had everything abstracted away and a single flag would have all past versions stored in a single json inside the cdk context file and automatically deployed.

comcalvi · 2022-05-13T16:45:32Z

ProductStack sits on top of the service itself, so what about the design of versioning in the service would make it impossible for CDK to leverage it? This seems to be an opportunity to simplify and reduce duplicated effort if we redesigned the ProductStack to leverage the new service versioning, instead of reimplementing it.

wanjacki · 2022-05-13T17:16:56Z

I see your point. This would depend on how Versioning is implemented in Service Catalog but it likely can be integrated with ProductStack. This is probably a big effort on Service Catalog side and would take time. Implementing it on CDK would be a quick win and would still allow the additional feature of disabling deployment from automatically overwriting of existing versions.

To add on to this. It still is a uniquely CDK issue as in Service Catalog versions are not overwritten at all. You can only just keep adding new versions or delete old versions manually. However in CDK, if we make code changes and deploy. The current state reflected in CDK will end up overwriting the current state of versions in Service Catalog.

comcalvi · 2022-05-13T18:06:33Z

Is it possible that the versioning in service catalog will break the versioning in CDK?

wanjacki · 2022-05-13T18:14:53Z

Is it possible that the versioning in service catalog will break the versioning in CDK?
That is a good point, I'm sure they have to maintain some sort of backwards compatibility with the existing Service Catalog.
However I will also work with them (the team working on it) to make sure it integrate itself well with CDK

skinny85 · 2022-05-13T18:15:07Z

packages/@aws-cdk/aws-servicecatalog/test/product.test.ts

+      });
+    }).toThrowError('Template MyProduct.ProductStack.v2.product.template.json cannot be found in product stack context');
+  }),
+


I feel like we're missing a test here for when versioningStrategy is RETAIN, but the ProductStack code chages between the time when the template was saved on disk, and then synthesized again? (That should result in an exception being thrown)

Pull request has been modified.

wanjacki · 2022-05-16T21:43:07Z

Since there was no update on comments, I went ahead with a commit that updated naming and documentation, moved the renamed RetentionStrategy to a field in fromProductStack and added the additional test. Let me know what you think.

rix0rrr · 2022-05-19T12:46:26Z

It is a bit unclear if we are deploying their currentVersion from productStack and all others from snapshot or if we are deploying all versions from snapshot and how locking would come in to play in both either of those cases.

It's up to us to decide what a good workflow is. We are developers, we can put ourselves in the shoes of another developer.

A developer workflow typically looks like this:

Type some code
Build/compile/test/run/deploy it
Does it look good?
- If no, go back to step 1
- If yes, commit and go do some other task

(Obviously there will probably be 2 such loops: one with the code on your developer desktop, and then another one with the code going through CI and running through the test environments).

Here's my take:

Setting the tip/head/current version to be immutable doesn't make sense because otherwise how will you iterate?
Not deploying the mutable tip/head/current doesn't make sense, because how will you test it otherwise?

That seems to imply to me: yes we deploy the current version from the ProductStack, and it should be mutable by default.

rix0rrr

Thanks for rolling with my proposal! I like this a lot better. I think we can move more logic into the History class though, and keep the other ones simple.

rix0rrr · 2022-05-19T12:48:56Z

packages/@aws-cdk/aws-servicecatalog/lib/cloudformation-template.ts

+  public static fromProductStackHistory(productStack: ProductStack, locked: boolean, directory?: string ): CloudFormationTemplate {
+    return new CloudFormationProductStackTemplate(productStack, locked, directory);
+  }
+
+  /**
+   * Creates a product from a previously deployed product stack snapshot.
+   */
+  public static fromProductStackSnapshot(productStack: ProductStack, directory?: string ): CloudFormationTemplate {
+    return new CloudFormationProductStackSnapshotTemplate(productStack, directory);
+  }


I think both of these are not necessary anymore, as long as productStackHistory.versionFromSnapshot('v1') can return a subclass of CloudFormationTemplate.

Okay I think we should be able to do that

rix0rrr · 2022-05-19T12:49:14Z

packages/@aws-cdk/aws-servicecatalog/README.md

+const productStackHistory = new servicecatalog.ProductStackHistory(this, 'ProductStackHistory', {
+  productStack: new S3BucketProduct(this, 'S3BucketProduct'),
+  currentVersionName: 'v2',
+  locked: true


After some consideration, I think currentVersionLocked would be a better name? WDYT?

Sounds good we can change it to be more clear.

rix0rrr · 2022-05-19T12:50:32Z

packages/@aws-cdk/aws-servicecatalog/lib/cloudformation-template.ts

+   * Creates a product with the resources defined in the given product stack and retains all previously deployed product stack versions.
+   */
+  public static fromProductStackHistory(productStack: ProductStack, locked: boolean, directory?: string ): CloudFormationTemplate {
+    return new CloudFormationProductStackTemplate(productStack, locked, directory);


locked and directory don't have to go in here. Either the current ProductStack is valid (in which case it's not necessary here), or it's not (in which case the build will fail anyway).

I think the issue is we use the locked and directory to determine if we need to write to snapshot and determine if we should deploy from snapshot as an asset or deploy it from our productStack.

If we are able to move the write logic (and possibly more logic) to the product-stack-history we might be able to remove this.

rix0rrr · 2022-05-19T12:51:19Z

packages/@aws-cdk/aws-servicecatalog/lib/cloudformation-template.ts

+  constructor(public readonly productStack: ProductStack,
+    public readonly locked?: boolean, public readonly directory?: string) {
+    super();


This class doesn't have to change.

rix0rrr · 2022-05-19T12:53:29Z

packages/@aws-cdk/aws-servicecatalog/lib/cloudformation-template.ts

+    productVersionDetails.locked = this.locked;
+    productVersionDetails.directory = this.directory;


I'm only now noticing you are doing this with mutability as intent.

Relying on side effects is a bad idea: it will make the data flow and dependencies between pieces of code VERY hard to reason about. Make as much as you can readonly please, and don't do this.

A better way to do this would have been below:

public bind(_scope: Construct): CloudFormationTemplateConfig { const productDetails = this.productStack._getProductVersionDetails(); return { httpUrl: this.productStack._getTemplateUrl(), productVersionDetails: { ...productDetails, locked: this.locked, directory: this.directory, }, templateType: TemplateType.PRODUCT_STACK, }; }

That's a general advice. In this particular case, don't modify this class at all.

I'll remove as many of these as possible. There is one case of productPathUniqueId that is only available when the Product is created and this value needs to be passed to ProductStack or ProductStackHistory to create a unique key for the file. The only link between the two is through cloudFormationTemplate, so I am unable to remove it in that instance (unless you have any ideas).

rix0rrr · 2022-05-19T12:54:34Z

packages/@aws-cdk/aws-servicecatalog/lib/common.ts

+   * ProductStackSnapshotTemplate
+   */
+  PRODUCT_STACK_SNAPSHOT = 'ProductStackSnapshotTemplate'


Is this still relevant? Isn't the snapshot template ultimately the same as an ASSET template?

Yes, there is a bit more logic in the SNAP_STACK_SNAPSHOT like a more detailed error when file is not found. Also currently ASSET (actual asset creation) is implemented in the bind method CloudFormationTemplate (path is passed by user).
I need to create the asset in the Product as we don't have access to the details to form the path in CloudFormationTemplate.

Yes, there is a bit more logic in the SNAP_STACK_SNAPSHOT like a more detailed error when file is not found.

What I'm saying is that if you do:

history.snapshotVersion('v1')

Then the implementation can look like:

class History { public snapshotVersion(version: string) { const snapshotFile = this.fileForVersion(version); if (!fileExists(snapshotFile)) { throw new Error('oh no you should have used this differently'); } return Template.fromAsset(snapshotFile); } }

The error message becomes part of the History class and can be as targeted and explicit as we need.

packages/@aws-cdk/aws-servicecatalog/lib/product-stack-history.ts

rix0rrr · 2022-05-19T12:55:35Z

packages/@aws-cdk/aws-servicecatalog/lib/product-stack-history.ts

+   */
+  readonly productStack: ProductStack;
+  /**


Empty lines between the property and the start of the next comment block please. Look at other code and follow the style there.

rix0rrr · 2022-05-19T12:57:05Z

packages/@aws-cdk/aws-servicecatalog/lib/product-stack-history.ts

+ */
+export interface ProductStackHistoryProps {
+  /**
+   * The base Product Stack.


"The base" ?

What does "base" mean? Please talk in terms that customer using your API will care and think about. Something like:

The ProductStack whose history will be snapshotted to files.

Or something to that effect.

rix0rrr · 2022-05-19T12:59:25Z

packages/@aws-cdk/aws-servicecatalog/lib/product-stack.ts

+    if (this._productVersionDetails.locked !== undefined) {
+      this.writeTemplateToSnapshot(cfn);
+    }


Why doesn't the History class write and validate the snapshots?

I think it could, it would involve giving the productStack access to it's History class and calling a method from it, I'll try to implement it.

comcalvi · 2022-05-19T17:15:59Z

packages/@aws-cdk/aws-servicecatalog/lib/product.ts

+          }
+          break;
+        case TemplateType.PRODUCT_STACK_CONTEXT:
+          const templateFileKey = `${this.productPathUniqueId}.${template.productVersionDetails?.productStackId}.${productVersion.productVersionName}.product.template.json`;


@rix0rrr is that because inheritance should be used instead? Could you elaborate?

packages/@aws-cdk/aws-servicecatalog/lib/product-stack-history.ts

Pull request has been modified.

wanjacki · 2022-05-19T18:30:48Z

@rix0rrr
Addressed all your comments, got rid of those static methods and moved more logic (validate and write) to ProductStackHistory.

rix0rrr

Again, very close! I'd prefer the change to be additive as much as possible.

Rather than changing and exposing implementation details of existing classes, I'd prefer to introduce new classes that conform to existing public contracts. Potentially it's okay to extend the existing contracts if new features need to be added, and it's definitely possible that we may need to change something in existing classes... but it shouldn't be the default option.

rix0rrr · 2022-05-23T12:11:43Z

packages/@aws-cdk/aws-servicecatalog/lib/cloudformation-template.ts

+/**
+ * Template from a previously deployed product stack.
+ */
+export class CloudFormationProductStackSnapshotTemplate extends CloudFormationTemplate {


Please don't expose this class publicly. I feel it can go into product-stack-history.ts as a private class.

rix0rrr · 2022-05-23T12:13:46Z

packages/@aws-cdk/aws-servicecatalog/lib/product-stack.ts

+  /**
+   * Fetch the product version details.
+   *
+   * @internal
+   */
+  public _getProductVersionDetails(): ProductVersionDetails {
+    return this._productVersionDetails;
+  }
+
+  /**
+   * Set the parent product stack history
+   *
+   * @internal
+   */
+  public _setParentProductStackHistory(parentProductStackHistory: ProductStackHistory) {
+    return this._parentProductStackHistory = parentProductStackHistory;
+  }
+


Please try to find a way to implement this feature without changing ProductStack (or convince me it's not possible).

rix0rrr · 2022-05-23T12:14:42Z

packages/@aws-cdk/aws-servicecatalog/lib/product.ts

+          }
+          break;
+        case TemplateType.PRODUCT_STACK_CONTEXT:
+          const templateFileKey = `${this.productPathUniqueId}.${template.productVersionDetails?.productStackId}.${productVersion.productVersionName}.product.template.json`;


It's because instanceof for type testing of custom classes is unreliable. It's too big of a topic to go in here, but we can talk about it in person if you want.

rix0rrr · 2022-05-23T12:16:30Z

packages/@aws-cdk/aws-servicecatalog/lib/product.ts

+          const templateFilePath = path.join(productStackSnapshotDirectory, templateFileKey);
+          if (!fs.existsSync(templateFilePath)) {
+            throw new Error(`Template ${templateFileKey} cannot be found in ${productStackSnapshotDirectory}`);
+          }
+          httpUrl = new s3_assets.Asset(this, `Template${hashValues(templateFileKey)}`, {
+            path: templateFilePath,
+          }).httpUrl;


This is tying the contents of the ProductStack class a little too much to where it's being used, in my mind.

This code could go into the bind() of CloudFormationProductStackSnapshotTemplate , right? Wouldn't that behave exactly the same, except we don't need to change this class so much?

What I'm looking for is roughly that the output of bind() is ~what goes into the productVersions list (modulo some information that may come from this construct or some simple transformation).

rix0rrr · 2022-05-23T12:32:53Z

packages/@aws-cdk/aws-servicecatalog/lib/product-stack.ts

+    if (this._parentProductStackHistory) {
+      this._parentProductStackHistory._writeTemplateToSnapshot(cfn);
+    }


I think this can be replaced with:

class ProductStackHistory { constructor(...) { Aspects.of(this).addAspect({ visit: (c) => { if (c === this) { this.writeTemplateToSnapshot(props.currentProductStack); } } }); } }

And that would get rid of most of the poking at the innards of existing classes.

Pull request has been modified.

wanjacki · 2022-05-23T23:19:25Z

@rix0rrr
Added a new revision addressing all your comments and isolated almost all changes to ProductStackHistory with the exception of that issue we discussed with _writeTemplateToSnapshot in ProductStack

There are now no changes to cloudformation-template.tsor product.ts.
I also removed the side effect for ProductPathUniqueId by using the unique id of our ProductStackHistory Construct for the file key.

rix0rrr

👍

mergify · 2022-05-24T06:37:25Z

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).

aws-cdk-automation · 2022-05-24T07:21:27Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 38bf88c
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-05-24T07:21:52Z

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).

padaszewski · 2022-05-31T10:26:06Z

@wanjacki
Thx for that, well done! We are probably implementing that in the next iteration if this suits for our purpose so if You just could answer me one question I would be very grateful.
Lets assume the following scenario: I have deployed a portfolio with a product with the productStackHistory feature.
In the target account the customer is launching the v1 of the product. In the meantime I deploy the v2 and inform the customer about that. The customer launches v2 (so updates v1->v2 with the same name etc). After that the customer encounters some breaking issues in the v2 and wants the v1 back for now, so he launches the v1 again. The question is: does this launch of the previous version causes the creation of a new CFN Stack, or a new Product (v1 and v2 lives in parallel?). If it would be the case we would have the problems with the namings and probably this launch would be rejected. So my expected behaviour would be that this v2 would be again overriden by memorized v1. Is this the case?
Thx in advance

wanjacki · 2022-05-31T15:02:30Z

@padaszewski
That depends on how the customer provisions the product.
They can change their existing provisioned product to a new version (or old version) which will be in the same CFN Stack
or they can have v1 and deploy v2 as a new provisioned product which will create a new Stack.

padaszewski · 2022-05-31T15:06:53Z

@wanjacki
Okay, cool. That means that as long as we provide a good documentation on this the customer can downgrade v2 to v1 in the same stack. Because of the naming conventions it is impossible to have two version of the product running in parallel in one account. As far as I understood our scenario with the downgrade can be covered by this history feature, right?

wanjacki · 2022-05-31T17:50:31Z

@padaszewski
I'm not sure what you mean by naming conventions makes it impossible to have two version of the product running in parallel in one account. You can provision the same version of a product twice in one account each with a different provisioned-product name.

Yes, you should be able to downgrade using one stack (if I understand your scenario correctly).

padaszewski · 2022-06-01T07:21:44Z

@wanjacki
Thx! That was also what I thought according the downgrade.

And about the naming conventions: the requirement is hat we have very specific and static bucket names in a certain account. For now this cannot be created in a dynamic manner. Thats why two stacks would fail to deploy.

padaszewski · 2022-06-01T16:31:01Z

@wanjacki
I introduced this concept to my collegues today and we plan to use this feature productive, but we have some more questions:
What exactly has to be tracked by version control?
If we have v1 LTS and the current version is eg v3 and we want to provide an update to v1 but this ProductStack obviously does not exist in the v1 state anymore. How we can do this update?

wanjacki · 2022-06-01T17:29:38Z

@padaszewski
Theproduct-stack-snapshots directory where previous versions are stored would hold a copy of the template for v1 even if the v1 state no long exist.
This directory would be tracked by version control as you can reference it using history.versionFromSnapshot('v1')

aws-cdk-automation requested a review from a team May 6, 2022 17:41

github-actions bot added the p2 label May 6, 2022

aws-cdk-automation assigned Naumel May 6, 2022

wanjacki force-pushed the productStackCache branch 2 times, most recently from 28059cb to 522ed7f Compare May 6, 2022 17:52

wanjacki force-pushed the productStackCache branch from 48166c3 to b4e67c6 Compare May 11, 2022 14:53

feat(servicecatalog): Add VersioningStrategy to retain previously dep…

6ab53e6

…loyed ProductStacks.

wanjacki force-pushed the productStackCache branch from b4e67c6 to 6ab53e6 Compare May 11, 2022 16:59

comcalvi previously requested changes May 12, 2022

View reviewed changes

rix0rrr previously requested changes May 13, 2022

View reviewed changes

rix0rrr reviewed May 13, 2022

View reviewed changes

skinny85 reviewed May 13, 2022

View reviewed changes

wanjacki changed the title ~~feat(servicecatalog): Add VersioningStrategy to retain previously deployed ProductStacks~~ feat(servicecatalog): Add RetentionStrategy to retain previously deployed ProductStacks May 16, 2022

wanjacki requested a review from rix0rrr May 16, 2022 21:47

wanjacki force-pushed the productStackCache branch from d2e2af0 to 90b18de Compare May 18, 2022 23:11

rix0rrr previously requested changes May 19, 2022

View reviewed changes

comcalvi previously requested changes May 19, 2022

View reviewed changes

wanjacki requested a review from rix0rrr May 19, 2022 18:30

Addressed comments, moved validate and write snapshot logic

e04d282

wanjacki force-pushed the productStackCache branch from f13f4d4 to e04d282 Compare May 19, 2022 19:48

rix0rrr previously requested changes May 23, 2022

View reviewed changes

Addressed comments, isolated almost all changes to ProductStackHistory

002c5cf

wanjacki requested a review from rix0rrr May 23, 2022 23:19

rix0rrr approved these changes May 24, 2022

View reviewed changes

Merge branch 'master' into productStackCache

38bf88c

rix0rrr changed the title ~~feat(servicecatalog): Add ProductStackHistory Construct to retain previously deployed ProductStacks~~ feat(servicecatalog): ProductStackHistory can retain old ProductStack iterations May 24, 2022

mergify bot merged commit 1037b8c into aws:master May 24, 2022

comcalvi added the effort/large Large work item – several weeks of effort label May 25, 2022

	export const PRODUCT_STACK_CONTEXT_DIRECTORY = 'product-stack-context';
	export const PRODUCT_STACK_CONTEXT_DIRECTORY = 'product-stack-history';

		productVersionDetails.locked = this.locked;
		productVersionDetails.directory = this.directory;

feat(servicecatalog): ProductStackHistory can retain old ProductStack iterations #20244

feat(servicecatalog): ProductStackHistory can retain old ProductStack iterations #20244

Conversation

wanjacki commented May 6, 2022

gitpod-io bot commented May 6, 2022

github-actions bot commented May 10, 2022

wanjacki commented May 10, 2022

github-actions bot commented May 11, 2022

robertd commented May 11, 2022

robertd commented May 11, 2022

robertd commented May 11, 2022

wanjacki commented May 11, 2022

comcalvi 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

rix0rrr May 18, 2022 • 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

wanjacki May 13, 2022 • 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

skinny85 May 13, 2022 • 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

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

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr May 13, 2022 • edited Loading

Choose a reason for hiding this comment

rix0rrr May 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr May 18, 2022 • 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

wanjacki commented May 13, 2022 • edited Loading

comcalvi commented May 13, 2022

wanjacki commented May 13, 2022 • edited Loading

comcalvi commented May 13, 2022

wanjacki commented May 13, 2022

Choose a reason for hiding this comment

wanjacki commented May 16, 2022 • edited Loading

rix0rrr commented May 19, 2022

rix0rrr 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

rix0rrr May 18, 2022 •

edited

Loading

wanjacki May 13, 2022 •

edited

Loading

skinny85 May 13, 2022 •

edited

Loading

rix0rrr May 13, 2022 •

edited

Loading

rix0rrr May 13, 2022 •

edited

Loading

rix0rrr May 18, 2022 •

edited

Loading

wanjacki commented May 13, 2022 •

edited

Loading

wanjacki commented May 13, 2022 •

edited

Loading

wanjacki commented May 16, 2022 •

edited

Loading

wanjacki May 19, 2022 •

edited

Loading