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

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.

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

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.

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.

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

we need a test for the default case as well.

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

This might be a sign of poor naming.

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

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

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.

?

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

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)

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

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

Sounds good we can change it to be more clear.