Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

Resource implementations for AWS::CloudFormation::ResourceVersion and AWS::CloudFormation::ResourceVersionAlias #4

Open
wants to merge 18 commits into
base: master
from

Conversation

@rjlohan
Copy link
Contributor

rjlohan commented Nov 20, 2019

Issue #, if available: #3

Description of changes:

Schema and handler implementations for AWS::CloudFormation::ResourceVersion and AWS::CloudFormation::ResourceVersionAlias.

NOTE: Unit tests are basic and pass, but I haven't yet run a submit/test cycle with CFN. Posting code for further discussion.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@rjlohan rjlohan self-assigned this Nov 20, 2019
@rjlohan rjlohan added the enhancement label Nov 20, 2019
Copy link

benkehoe left a comment

This falls into the same trap that Lambda has where you can't create two versions in a single stack.

"description": "When the specified type version was registered.",
"type": "string"
},
"Type": {

This comment has been minimized.

Copy link
@benkehoe

benkehoe Nov 20, 2019

Why can't this be moved from a property of the resource to be the actual resource type? AWS::CloudFormation::Resource?

This comment has been minimized.

Copy link
@rjlohan

rjlohan Nov 21, 2019

Author Contributor

Definitely could, but at this point I don't think there's anything about this model which is resource specific and I don't foresee anything like that coming in. But that sort of thinking led to AWS::RDS::DBInstance which I really want to refactor.

"pattern": "^arn:aws[A-Za-z0-9-]{0,64}:cloudformation:[A-Za-z0-9-]{1,64}:([0-9]{12})?:type/.+$",
"type": "string"
},
"DefaultVersionId": {

This comment has been minimized.

Copy link
@benkehoe

benkehoe Nov 20, 2019

It's strange to have the DefaultVersion on the resource representing a different version from the version that the resource itself represents. I think there are two non-exclusive options:

  • There's a property on the resource that enables the "update default version" behavior
  • There's a separate resource type for aliases, of which there's only the "default" today.

This comment has been minimized.

Copy link
@rjlohan

rjlohan Nov 21, 2019

Author Contributor

If we had this property on the Type resource itself I think it would be a readOnlyProperties annotation for use in Drift Detection type scenarios.

This comment has been minimized.

Copy link
@aygold92

aygold92 Nov 26, 2019

Contributor

this is also a ReadOnly property -- we want to provide all the properties in the desribe-type API output

This comment has been minimized.

Copy link
@benkehoe

benkehoe Nov 27, 2019

It wasn't indicated as ReadOnly in the example @rjlohan provided in #3, it was suggested that you'd be able to set the default version through this same resource.

Also, note that while I don't think having both API calls in the same resource is a good idea in this case (see below), I specifically advocate discarding the notion that a resource should correspond exactly to a single API call.

],
"readOnlyProperties": [
"/properties/Arn",
"/properties/TypeVersionArn",

This comment has been minimized.

Copy link
@benkehoe

benkehoe Nov 20, 2019

Shouldn't there also be a VersionId so that it could be copied for use with DefaultVersionId?

This comment has been minimized.

Copy link
@rjlohan

rjlohan Nov 21, 2019

Author Contributor

Possibly, yes. Think we need to work out if this resource should model the Type or the TypeVersion, I think the way it will be used will be slightly different depending on which way we go.

This comment has been minimized.

Copy link
@aygold92

aygold92 Nov 26, 2019

Contributor

The way the schema was written, it was meant to model Type

This comment has been minimized.

Copy link
@benkehoe

benkehoe Nov 27, 2019

Except that with every update, it creates a new TypeVersion, which we need to be able to discover and reference, for example to set the default version.

"minLength": 1,
"type": "string"
},
"SchemaHandlerPackage": {

This comment has been minimized.

Copy link
@benkehoe

benkehoe Nov 20, 2019

Should this have a more specific schema given that it is supposed to be an S3 bucket URL? Can it reference the S3 Bucket resource schema?

This comment has been minimized.

Copy link
@rjlohan

rjlohan Nov 21, 2019

Author Contributor

Yes that's the eventual intent, we just have to finish up the schema hosting work so that other schemas are accessible via URI. Not far off, and then we can update the existing schemas with the correct $ref pointer for these sort of properties.

@pgarbe

This comment has been minimized.

Copy link

pgarbe commented Nov 21, 2019

What if you rename Type into TypeVersion and introduce another resource called Type which just refers to an existing TypeVersion?

So you can have multiple TypeVersions to register different versions and only one Type for a specific TypeName.

Although, I'm not really aware of an use case that needs multiple versions registered.

MyType:
  Type: AWS::CloudFormation::Type
  Properties:
    Version: !Ref MyTypeVersion1
    TypeName: Organisation::Service::Resource
    ExecutionRoleArn: arn:aws:iam::123456789012:role/MyExecutionRole
    LoggingConfiguration:
        LogRoleArn: arn:aws:iam::123456789012:role/MyLoggingRole
        LogGroupName: MyResourceLogGroups  

MyTypeVersion1:
  Type: AWS::CloudFormation::TypeVersion
  Properties:
    Kind: Resource
    TypeName: Organisation::Service::Resource
    DefaultVersion: "00000001"
    SchemaHandlerPackage: my-artifact-bucket
@benkehoe

This comment has been minimized.

Copy link

benkehoe commented Nov 21, 2019

In my ideal world, it's three resource types, AWS::CloudFormation::Resource, AWS::CloudFormation::ResourceVersion, and AWS::CloudFormation::ResourceDefaultVersion/AWS::CloudFormation::ResourceVersionAlias. The template would look like:

MyResource:
  Type: AWS::CloudFormation::Resource
  Properties:
    TypeName: Organisation::Service::Resource

MyResourceVersion:
  Type: AWS::CloudFormation::ResourceVersion
  Properties:
    ResourceType: !Ref MyResource
    ExecutionRoleArn: arn:aws:iam::123456789012:role/MyExecutionRole
    LoggingConfiguration:
      LogRoleArn: arn:aws:iam::123456789012:role/MyLoggingRole
      LogGroupName: MyResourceLogGroups  
    SchemaHandlerPackage: my-artifact-bucket

MyResourceDefaultVersion:
  Type: AWS::CloudFormation::ResourceDefaultVersion
  Properties:
    ResourceType: !Ref MyResource
    DefaultVersion: !GetAtt MyResourceVersion.VersionId

This would allow multiple versions to be defined in a single template. This may become important when versioning gains more semantics (aliases representing major versions). I take the view that if you can't represent any state of the service achievable through the console in a single template, the resource model is broken. Another benefit is that resource versions would get deregistered individually as the resources that represent them get deleted, which would help in the case that, for example, credentials get accidentally hard coded in the source rather than stored externally.

That said, it's verbose, and maybe there's a way to define AWS::CloudFormation::Resource such that it could take the version config inline, and later be extended to support the placeholder usage with a separate ResourceVersion that I've outlined above. I do think the default version should exist as a separate resource from the start, though.

@rjlohan

This comment has been minimized.

Copy link
Contributor Author

rjlohan commented Nov 21, 2019

Thanks for the feedback @pgarbe and @benkehoe. Let's experiment with these ideas. I'm going to be somewhat offline for a couple of days, but I'll aim to try and bootstrap the repo with API calls and setup so we can see how this plays out. Or if one of you want to have a crack at a PR, you could refer to one of the working examples like AWS::Logs::LogGroup for some of the conventions we use as a starting point.

@rjlohan

This comment has been minimized.

Copy link
Contributor Author

rjlohan commented Nov 27, 2019

This falls into the same trap that Lambda has where you can't create two versions in a single stack.

This is actually not possible in the underlying APIs anyway; one registration is allowed to be in flight at the same time, and (currently) only one version can be set as the Default (i.e; the one which CloudFormation will invoke when this type is specified in a template)

@benkehoe

This comment has been minimized.

Copy link

benkehoe commented Nov 27, 2019

This is actually not possible in the underlying APIs anyway

Sure. And you'll argue that since you can't deregister a type unless it has only one version, even though a type with lots of versions would require a long sequence of stack updates to restore, deregistering it by accident is unlikely and so we don't need to account for that case. And so given the API I guess it's probably ok to leave it without multiple version representation. But I do want to register my disappointment that even on the CloudFormation team you're not starting with CloudFormation resource design, ensuring proper infrastructure as code lifecycle management, and working backwards from there to API design.

The fact that there can be only one default would point to DefaultVersion being set on the AWS::CloudFormationResource resource itself, but it still feels weird to have it potentially point to a different version that is represented by the resource in the stack (i.e., the current version). So I guess I think I would propose the either/or scenario I mentioned above:

  • An optional SetDefaultToLatest: True property on the AWS::CloudFormation::Resource. When this is set the version represented by the resource is also the default, so there isn't that disconnect
  • A separate AWS::CloudFormation::ResourceDefaultVersion or AWS::CloudFormation::ResourceVersionAlias resource that can either !GetAtt the current resource version or set it to a specific value.
    It would be up to the user to not do both at the same time. CloudFormation linting could help with this.
@pgarbe

This comment has been minimized.

Copy link

pgarbe commented Nov 27, 2019

This is actually not possible in the underlying APIs anyway; one registration is allowed to be in flight at the same time, and (currently) only one version can be set as the Default (i.e; the one which CloudFormation will invoke when this type is specified in a template)

That's even more confusing. From the CLI, I'd assume that you can register multiple versions for a type and point the "default" to one of the registered versions. Can you elaborate what you mean by this is not possible?

@benkehoe

This comment has been minimized.

Copy link

benkehoe commented Nov 27, 2019

From the CLI, I'd assume that you can register multiple versions for a type and point the "default" to one of the registered versions.

That's true. What Ryan was saying is that you can't have two simultaneous RegisterType calls trying to create new versions at the same time, which is what might happen if there is a resource for AWS::CloudFormation::ResourceVersion and you've got two of those in the same template, which would be possible under my proposed design on purpose (I want this kind of functionality for Lambda resources).

* AWS::CloudFormation::Type defines the primary behaviour for provisioning a CloudFormation Type
* AWS::CloudFormation::TypeVersion defines the mechanism for specifying the default version of an AWS::CloudFormation::Type
  - Create defers to Update for implementation
  - Read returns essentially the same input model, but can be used to verify existence and ensure that the default version has not changed on a type for Drift Detection
  - Update invokes SetDefaultTypeVersion API for implementation
  - Delete is a no-op and resource will simply be removed from the CloudFormation Stack
  - List is not supported
@rjlohan rjlohan changed the title First pass at a proposed resource schema for AWS::CloudFormation::Type First pass at a proposed resource schema for AWS::CloudFormation::Type and AWS::CloudFormation::TypeVersion Dec 11, 2019
@rjlohan

This comment has been minimized.

Copy link
Contributor Author

rjlohan commented Dec 11, 2019

FYI I've added a basic implementation against this schema, haven't tested it yet. Just wanting to show progress and that I haven't walked away. :)

Also - build is failing because I didn't meet my own coverage bar (yet). 💔

Copy link
Contributor

aygold92 left a comment

This immutable update style of this implementation won't work, because CloudFormation won't let you create a second resource with the same primaryIdentifier (which is Type/TypeName). if you ever try to update it, the update will fail because the type/typeName already exists.

We can solve this by adding an update handler which basically does the same thing as create (but doesn't need to check for existence). The only problem then (aside from hiding versions, which may or may not be desired) is that during delete we would probably have to list and then deregister all type versions.

try {
proxy.injectCredentialsAndInvokeV2(Translator.translateToCreateRequest(model),
ClientBuilder.getClient()::registerType);
} catch (final CfnRegistryException e) {
throw new CfnGeneralServiceException(e);
}
Comment on lines 83 to 88

This comment has been minimized.

Copy link
@aygold92

aygold92 Dec 11, 2019

Contributor

we could also catch ValidationException and return InvalidRequest

This comment has been minimized.

Copy link
@aygold92

aygold92 Dec 11, 2019

Contributor

Other easy one would be to check for status code > 500 and return ServiceInternalError

we probably also want to set a good example here and write some mapping for all the rest of the error codes. AccessDenied, InvalidCredentials, Throttling, etc.

} catch (final TypeNotFoundException e) {
throwNotFoundException(model);
} catch (final CfnRegistryException e) {
throw new CfnGeneralServiceException("DeregisterType: " + e.getMessage());

This comment has been minimized.

Copy link
@aygold92

aygold92 Dec 11, 2019

Contributor

nit: something a little more customer friendly like "Failed to deregister type: %s"

.callbackContext(context)
.callbackDelaySeconds(5)
.status(OperationStatus.IN_PROGRESS)
.resourceModel(request.getDesiredResourceState())

This comment has been minimized.

Copy link
@aygold92

aygold92 Dec 11, 2019

Contributor

nice. Because the Type/TypeName is the physicalId, this will satisfy the physicalId requirement, and because we don't return until we've successfully registered, we know that there is in fact a registration in progress.

One scenario that could cause leaking is if the describe call fails for some reason, but we can iterate on that as it comes up


static DescribeTypeRequest translateToReadRequest(@NonNull final ResourceModel model) {
return DescribeTypeRequest.builder()
.typeName(model.getTypeName())

This comment has been minimized.

Copy link
@aygold92

aygold92 Dec 11, 2019

Contributor

not totally sure, but I think since you're only specifying the typeName instead of the typeArn, Type RESOURCE is required

@rjlohan

This comment has been minimized.

Copy link
Contributor Author

rjlohan commented Dec 19, 2019

So the PR currently 'works' here, except that the AWS::CloudFormation::TypeVersion is unable to get the VersionId from a !GetAtt to an AWS::CloudFormation::Type, due to a problem with the DescribeType API which I am going to fix in the Registry. You should be able to hardcode a version ID though, and get these 2 packages to work against CFN.

This template snippet demonstrates usage (you need your own code packages and S3 object links - make sure the code is actually different in each package or you'll eventually run into a separate bug which we need to fix);

Resources:
    InitialType:
        Type: AWS::CloudFormation::Type
        Properties:
            Type: RESOURCE
            TypeName: Sample::CloudFormation::Resource
            SchemaHandlerPackage: s3://cloudformationmanageduploadinfrast-artifactbucket-123456789012abcdef/sample-cloudformation-resource.zip
    UpdatedType:
        Type: AWS::CloudFormation::Type
        Properties:
            Type: RESOURCE
            TypeName: Sample::CloudFormation::Resource
            SchemaHandlerPackage: s3://cloudformationmanageduploadinfrast-artifactbucket-123456789012abcdef/sample-cloudformation-resource-update.zip
        DependsOn: InitialType

    DefaultVersion:
        Type: AWS::CloudFormation::TypeVersion
        Properties:
            Arn: !Ref UpdatedType
           #  DefaultVersion: !GetAtt UpdatedType.VersionId
            DefaultVersion: 00000002

And if you are interested, the issue blocking !GetAtt from working is that the CloudFormation DescribeType API returns the DefaultVersionId when called for a Type ARN, but does not indicate anything about the DefaultVersionId when called for a Type Version ARN, and this makes implementing Read correctly a giant pain.

@benkehoe

This comment has been minimized.

Copy link

benkehoe commented Dec 19, 2019

I like this resource layout, but I think the names should be changed to reflect the shift in responsibility. I'd propose AWS::CloudFormation::TypeVersion and AWS::CloudFormation::TypeVersionAlias. So it would look like this:

Resources:
    InitialType:
        Type: AWS::CloudFormation::TypeVersion
        Properties:
            Type: RESOURCE
            TypeName: Sample::CloudFormation::Resource
            SchemaHandlerPackage: s3://cloudformationmanageduploadinfrast-artifactbucket-123456789012abcdef/sample-cloudformation-resource.zip
    UpdatedType:
        Type: AWS::CloudFormation::TypeVersion
        Properties:
            Type: RESOURCE
            TypeName: Sample::CloudFormation::Resource
            SchemaHandlerPackage: s3://cloudformationmanageduploadinfrast-artifactbucket-123456789012abcdef/sample-cloudformation-resource-update.zip
        DependsOn: InitialType

    DefaultVersion:
        Type: AWS::CloudFormation::TypeVersionAlias
        Properties:
            Arn: !Ref UpdatedType
            AliasName: $default # only valid value currently
            # VersionId: !GetAtt UpdatedType.VersionId
            VersionId: 00000002

I'd argue for a) having an alias name that required to be a specific value, rather than making default version different from possible future aliases from the IaC perspective (templates don't need to match the API calls) and b) going with $default as the alias name as that's what Lambda and API Gateway are using in their alias naming.
As to VersionId: is it a string or a number? Leading zeros are hard in YAML (see @rhboyd's tribulations with account IDs in mappings).

I'm sad that it's AWS::CloudFormation::TypeVersion+Type=RESOURCE and not AWS::CloudFormation::ResourceVersion. I get that there will be different types in the future, but I expect that they will be managed by users differently, have different lifecycles, etc., so I having been hoping that they could be different CloudFormation types. If they were separate CloudFormation types, when you release a new registry type, say it's called "Foo", a generic CloudFormation analyzer would know "this template defines M new Resources and N new Foos and K IAM roles and..." without knowing anything about the properties of the resources. Under this scheme, it would only be able to say "this template defines X CloudFormation types 🤷‍♂️ and K IAM roles and...", and while X=M+N, it would take additional specialized code to split that out.

I get that this will save duplication of code between the resources and matches the API, but it feels like optimizing this handler implementation at the expense of the user experience.

@rjlohan

This comment has been minimized.

Copy link
Contributor Author

rjlohan commented Dec 19, 2019

I'm sad that it's AWS::CloudFormation::TypeVersion+Type=RESOURCE and not AWS::CloudFormation::ResourceVersion.

I could probably be convinced that this is OK. You're right that I'm optimizing for implementation detail here and that's probably the wrong approach. Can always share 99% of the code behind slightly different schemas if needed. There's a bit of YAGNI here too and I should worry about what we have more than what we may have later. 😄

@benkehoe

This comment has been minimized.

Copy link

benkehoe commented Dec 19, 2019

Oh, fabulous! So you can be convinced--what do I need to do to convince you?

@rjlohan

This comment has been minimized.

Copy link
Contributor Author

rjlohan commented Dec 19, 2019

Oh, fabulous! So you can be convinced--what do I need to do to convince you?

Send beer. 👌

@benkehoe

This comment has been minimized.

Copy link

benkehoe commented Dec 20, 2019

I tried to buy you all beer at re:Invent for exactly this kind of eventuality but Amjad wouldn't let me

@rjlohan

This comment has been minimized.

Copy link
Contributor Author

rjlohan commented Dec 20, 2019

Latest revision has the new type names as suggested. Still a problem on setting the DefaultVersionId that I have to fix up.

@rjlohan rjlohan changed the title First pass at a proposed resource schema for AWS::CloudFormation::Type and AWS::CloudFormation::TypeVersion Resource implementations for AWS::CloudFormation::ResourceVersion and AWS::CloudFormation::ResourceVersionAlias Dec 20, 2019
@benkehoe

This comment has been minimized.

Copy link

benkehoe commented Jan 13, 2020

Pinging on my question about DefaultVersion: 00000002 in your resource example. I have a hard time understanding what an eventual non-default alias would look like, since it must presumably have a name and VersionId, so I imagine something like:

MyVersion:
    Type: AWS::CloudFormation::TypeVersionAlias
    Properties:
        Arn: !Ref UpdatedType
        AliasName: SomeAliasName
        # VersionId: !GetAtt UpdatedType.VersionId
        VersionId: 00000002

Given that, it seems like the VersionId field should be the same whether it's a default or "custom" alias. So then the default alias could either reuse AliasName with a special value like $default (what Lambda and API Gateway use), or something like IsDefaultAlias: true

@rjlohan

This comment has been minimized.

Copy link
Contributor Author

rjlohan commented Jan 14, 2020

Pinging on my question about DefaultVersion: 00000002 in your resource example. I have a hard time understanding what an eventual non-default alias would look like, since it must presumably have a name and VersionId, so I imagine something like:

MyVersion:
    Type: AWS::CloudFormation::TypeVersionAlias
    Properties:
        Arn: !Ref UpdatedType
        AliasName: SomeAliasName
        # VersionId: !GetAtt UpdatedType.VersionId
        VersionId: 00000002

Given that, it seems like the VersionId field should be the same whether it's a default or "custom" alias. So then the default alias could either reuse AliasName with a special value like $default (what Lambda and API Gateway use), or something like IsDefaultAlias: true

Yeah that makes sense. I'm still fixated on the current nomenclature having only a Default. I'll probably rework this - consistency with other similar types like AWS::Lambda::Alias is a good way to go.

Right now I'm blocked on completion by a change I need to make on the backend - which I've done, just needs to be rolled out.

rjlohan added 2 commits Feb 11, 2020
@rjlohan rjlohan force-pushed the rjlohan:aws-cloudformation-type branch from cdb7689 to b5a2753 Feb 12, 2020
@ikben

This comment has been minimized.

Copy link

ikben commented Feb 22, 2020

I'm not sure if this is already provided by the code/rpdk (my java is very rusty), but to come back on:

DefaultVersion: !GetAtt UpdatedType.VersionId
DefaultVersion: "00000002"
DefaultVersion: 00000002

I think it might make sense to actually fail and give an error message in the case that the supplied version is a number (even if the API would be able to set the default version.

  • I expect that in most cases the DefaultVersion will be a GetAtt or a Ref to a Parameter, in which case it doesn't matter
  • In the case that it's not it will work fine for 00000001 to 00000009, but suddenly fail on 00000010 (or worse, revert to the old version, "00000008" if it sill exists). Which will lead to frustration and/or bugs
  • Throwing errors on numbers will be annoying the first time someone runs into it, but I think its preferable to the alternative
  • Accepting numbers now will mean accepting them forever to prevent breaking templates. Rejecting numbers is a two-way door.
@benkehoe

This comment has been minimized.

Copy link

benkehoe commented Feb 22, 2020

It's definitely unclear whether version numbers are strings or numbers, and whether or not the leading zeros are required. If they are...are you sure you have enough? I might need 100 million versions, I am constantly screwing things up 😜

@ikben

This comment has been minimized.

Copy link

ikben commented Feb 24, 2020

@pgarbe

This comment has been minimized.

Copy link

pgarbe commented Feb 24, 2020

Did I get it right that the type AWS::CloudFormation::ResourceVersion is immutable and can't be changed after it's created. So, if I want to provide a newer implementation I've to create a second resource in my template and change the default version. Once that is deployed, I can remove the first version and deploy again. Right?

I'm not sure if makes sense to compare it with Lambda Version/Alias. A typical use case that I've in mind is to just change the package version that is provided by the Vendor. So my first deployment is:

Resources:
    MySampleResource:
        Type: AWS::CloudFormation::Resource
        Properties:
            TypeName: Sample::CloudFormation::Resource
            SchemaHandlerPackage: s3://cloudformationmanageduploadinfrast-artifactbucket-123456789012abcdef/sample-cloudformation-resource-v0.0.1.zip

and in the second deployment, I just change the package location:

Resources:
    MySampleResource:
        Type: AWS::CloudFormation::Resource
        Properties:
            TypeName: Sample::CloudFormation::Resource
            SchemaHandlerPackage: s3://cloudformationmanageduploadinfrast-artifactbucket-123456789012abcdef/sample-cloudformation-resource-v0.0.2.zip

Everything else should be handled by CloudFormation and as user I don't care how many and which API calls are necessary. And in that case I wouldn't call it ResourceVersion but just Resource.

@benkehoe

This comment has been minimized.

Copy link

benkehoe commented Feb 24, 2020

You can do exactly what you are describing with AWS::CloudFormation::ResourceVersion. The semantics are that updates to SchemaHandlerPackage require replacement, so when you've changed that property, a new version will be created and the old version deleted. The reason why it's ::ResourceVersion is that it represents a specific version of the resource type, rather than the resource type as a whole (note that if you !Ref it, it would be an ARN to the version), and in particular, you can have more than one ::ResourceVersion of the same resource type in the same template (for example, if you're supporting a "v1.x" for one team and "v2.x" for another team). This is something that isn't possible with Lambda. For example, you can't deploy two versions of a Lambda function and a weighted alias with references to both in a single template. There's a bit more explanation on this topic in this article.

The primary difference with Lambda is that with resource types, there's never a mutable $LATEST version anyway.

@pgarbe

This comment has been minimized.

Copy link

pgarbe commented Feb 24, 2020

Thanks @benkehoe for clarification. So both use cases are supported which is perfect. Now it just needs to be shipped...

@benkehoe

This comment has been minimized.

Copy link

benkehoe commented Feb 24, 2020

We've still got some questions about ::ResourceVersionAlias to work out:

  • How is the default alias represented?
  • Are version ids numbers or strings (or both)?
@rjlohan

This comment has been minimized.

Copy link
Contributor Author

rjlohan commented Feb 24, 2020

  • How is the default alias represented?

I'll come back to this when I get the SDK update for the missing field.

  • Are version ids numbers or strings (or both)?

They are strings representing for 8 digit incrementing integers. 🤷‍♂

@benkehoe

This comment has been minimized.

Copy link

benkehoe commented Feb 24, 2020

They are strings representing for 8 digit incrementing integers. 🤷‍♂

I guess what I mean is, is the zero padding is required or optional? The SetTypeDefaultVersion API specifies the pattern to be [A-Za-z0-9-]+, which is of no help as letters are clearly not valid at present.

@rjlohan

This comment has been minimized.

Copy link
Contributor Author

rjlohan commented Feb 24, 2020

They are strings representing for 8 digit incrementing integers. 🤷‍♂

I guess what I mean is, is the zero padding is required or optional? The SetTypeDefaultVersion API specifies the pattern to be [A-Za-z0-9-]+, which is of no help as letters are clearly not valid at present.

The version string should be treated as a string, as we reserve the right to change their format in the future. So, the "0" padding is required because they're not numbers, they're strings.

@benkehoe

This comment has been minimized.

Copy link

benkehoe commented Mar 25, 2020

@rjlohan and I talked about this a few weeks ago. I came around to the idea that ::ResourceDefaultVersion should be separate from a future ::ResourceVersionAlias. In particular, ::ResourceVersionAlias would represent something like "1.x" versus "2.x" and would point to actual versions, but the ::ResourceDefaultVersion could point to a resource version alias (so you'd be able to say that by default people should be on 2.x). I do think the field should be VersionId, though. So:

    DefaultVersion:
        Type: AWS::CloudFormation::ResourceDefaultVersion
        Properties:
            Arn: !Ref UpdatedType
            #  VersionId: !GetAtt UpdatedType.VersionId
            VersionId: 00000002

Future version aliases could look like this:

    VersionAlias:
        Type: AWS::CloudFormation::ResourceVersionAlias
        Properties:
            Arn: !Ref UpdatedType
            AliasName: "2.x"
            #  VersionId: !GetAtt UpdatedType.VersionId
            VersionId: 00000002

    DefaultVersion:
        Type: AWS::CloudFormation::ResourceDefaultVersion
        Properties:
            Arn: !Ref UpdatedType
            VersionAlias: !Ref VersionAlias
@rjlohan

This comment has been minimized.

Copy link
Contributor Author

rjlohan commented Mar 25, 2020

@rjlohan and I talked about this a few weeks ago. I came around to the idea that ::ResourceDefaultVersion should be separate from a future ::ResourceVersionAlias. In particular, ::ResourceVersionAlias would represent something like "1.x" versus "2.x" and would point to actual versions, but the ::ResourceDefaultVersion could point to a resource version alias (so you'd be able to say that by default people should be on 2.x). I do think the field should be VersionId, though. So:

    DefaultVersion:
        Type: AWS::CloudFormation::ResourceDefaultVersion
        Properties:
            Arn: !Ref UpdatedType
            #  VersionId: !GetAtt UpdatedType.VersionId
            VersionId: 00000002

Future version aliases could look like this:

    VersionAlias:
        Type: AWS::CloudFormation::ResourceVersionAlias
        Properties:
            Arn: !Ref UpdatedType
            AliasName: "2.x"
            #  VersionId: !GetAtt UpdatedType.VersionId
            VersionId: 00000002

    DefaultVersion:
        Type: AWS::CloudFormation::ResourceDefaultVersion
        Properties:
            Arn: !Ref UpdatedType
            VersionAlias: !Ref VersionAlias

Thanks for writing that up, I would have forgotten. :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.