aws-cloudformation / aws-cloudformation-resource-providers-cloudformation

The CloudFormation Resource Provider Package For AWS CloudFormation
https://aws.amazon.com/cloudformation/
Apache License 2.0
48 stars 35 forks source link

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

Closed rjlohan closed 4 years ago

rjlohan commented 5 years ago

Issue #, if available: #3

Description of changes:

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

Sample template for use of these type;

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

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

Pending contract test verification.

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

pgarbe commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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:

pgarbe commented 5 years ago

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 commented 5 years ago

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

rjlohan commented 4 years ago

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). πŸ’”

rjlohan commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

rjlohan commented 4 years ago

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

Send beer. πŸ‘Œ

benkehoe commented 4 years ago

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

rjlohan commented 4 years ago

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

benkehoe commented 4 years ago

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 commented 4 years ago

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.

benbridts commented 4 years ago

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.

benkehoe commented 4 years ago

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 😜

benbridts commented 4 years ago

The string/number uncertainty would be consistent with the rest of the cfn service though

Almost consistent. IIRC Elastic Beanstalk Option Settings fail if the values are not strings.

pgarbe commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

benkehoe commented 4 years ago

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

rjlohan commented 4 years ago
  • 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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. :-/

pgarbe commented 4 years ago

Any updates here?

wolfeidau commented 4 years ago

Just started using resource providers and really keen on seeing this PR progress, is there anything we can do to help?

richardhboyd commented 4 years ago

One capability that I’d like to see in this is being able to register a new type and use it in the same template. Current validation mechanisms would fail because the type is unrecognized when stack creation starts.

rjlohan commented 4 years ago

One capability that I’d like to see in this is being able to register a new type and use it in the same template. Current validation mechanisms would fail because the type is unrecognized when stack creation starts.

Not a bad idea, but it's probably not something that can be achieved by this implementation. As you note - for the first registration the type won't exist when CloudFormation validates the existence of types up front. And beyond that, while the type would exist at that point, CloudFormation would use the previously registered version for the stack operation, rather than fetch this new one.

I don't have a clear picture on how that request might be solved without breaking this validation flow, but open to suggestions.

rjlohan commented 4 years ago

Just started using resource providers and really keen on seeing this PR progress, is there anything we can do to help?

Thanks but it's really just waiting on an SDK update with a required property, which is in progress.

benbridts commented 4 years ago

One capability that I’d like to see in this is being able to register a new type and use it in the same template. Current validation mechanisms would fail because the type is unrecognized when stack creation starts.

I understand that need, but I personally consider that an anti-pattern when used with Custom Resources, so I would be careful when adding this.

Macros (inherently) have the same limitation, and I believe that's the better model to follow everywhere (no template can change it's own runtime behaviour). This does raise an interesting question about what will happen if you switch versions while a template is in progress.

rjlohan commented 4 years ago

This does raise an interesting question about what will happen if you switch versions while a template is in progress.

When a stack operation starts, CloudFormation grabs the default version of all resources used in the template and 'pins' them for the remainder of that stack operation. This is how type versioning has always been handled in CloudFormation, albeit with different implementation details.

richardhboyd commented 4 years ago

The version is frozen when the stack operation starts? Does that mean various nested templates might use a different versions of a type of that type is updated out-of-band (by the CLI or another CFN Template)?

rjlohan commented 4 years ago

The version is frozen when the stack operation starts? Does that mean various nested templates might use a different versions of a type of that type is updated out-of-band (by the CLI or another CFN Template)?

No, they should use the same version too as the version for the type is already captured up front. The only time distortion you might see is that if a nested template contains a type that hasn't been used yet, it might use a version that was registered after the stack operation started.

richardhboyd commented 4 years ago

Suppose I have 3 stacks, one root (StackA) and two sibling nested stacks (StackB and StackC). StackA doesn't use MyType at all, but StackB and StackC both use it. (Let's also suppose StackC has an explicit DependsOn for StackB). Are you saying that when StackB uses MyType then the version is locked and StackC will use the same version?

richardhboyd commented 4 years ago

oof, I just realized I added my comment on the PR instead of the issue. feel free to ignore my comments here.

rjlohan commented 4 years ago

Suppose I have 3 stacks, one root (StackA) and two sibling nested stacks (StackB and StackC). StackA doesn't use MyType at all, but StackB and StackC both use it. (Let's also suppose StackC has an explicit DependsOn for StackB). Are you saying that when StackB uses MyType then the version is locked and StackC will use the same version?

That's how it should work, yes.

rjlohan commented 4 years ago

Just started using resource providers and really keen on seeing this PR progress, is there anything we can do to help?

Thanks but it's really just waiting on an SDK update with a required property, which is in progress.

Guess I'm out of excuses now: https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/pull/267

πŸ˜…

rjlohan commented 4 years ago

@benkehoe @richardhboyd @ikben @pgarbe @wolfeidau I have incorporated the SDK updates and done some basic end-to-end testing. If you are willing and able to provide some usabililty feedback or spot bugs, the current code can be cloned/forked and provisioned as a private type.

Appreciate any feedback you can give while I polish the tests and what not.

benkehoe commented 4 years ago

It looks like these changes aren't present yet: https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-cloudformation/pull/4#issuecomment-603902255

rjlohan commented 4 years ago

It looks like these changes aren't present yet: #4 (comment)

No, but imagine ResourceTypeAlias is renamed to ResourceDefaultVersion and the inline property is just part of the build. I can do that later if the usability pans out correctly.

There's no such thing as a named alias in the service so there's no way to implement this at the moment, except as a template construct with no backing resource.

rjlohan commented 4 years ago

Don't stress too much over a code review here, I have to do some non-logic-affecting rewrites based on latest Java Plugin updates, just want some usability feedback on the types.

rjlohan commented 4 years ago

Updated the implementation so that example usage looks like;

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

    DefaultVersion:
        Type: AWS::CloudFormation::ResourceDefaultVersion
        Properties:
            Arn: !Ref UpdatedType
rjlohan commented 4 years ago

I'm going to work through some additional testing on this, but I'm settled on this model now unless there's other critical feedback.

benkehoe commented 4 years ago

Arn: !Ref UpdatedType

Ohh, brilliant. Because there's no separate Resource to reference, why should there be separate Type and VersionId fields, you're just referencing the ResourceVersion. I love it.

Is the DependsOn necessary because there might be cross-talk if they get created at the same time? Otherwise, it shouldn't matter which one gets created first, right? Is this something https://github.com/aws-cloudformation/aws-cloudformation-resource-schema/issues/79 would help with?

rjlohan commented 4 years ago

Arn: !Ref UpdatedType

Ohh, brilliant. Because there's no separate Resource to reference, why should there be separate Type and VersionId fields, you're just referencing the ResourceVersion. I love it.

Yep, but I've allowed both options in case you're managing across stacks or something. I'd expect !Ref UpdatedType to be the usual usage though.

Is the DependsOn necessary because there might be cross-talk if they get created at the same time? Otherwise, it shouldn't matter which one gets created first, right? Is this something aws-cloudformation/aws-cloudformation-resource-schema#79 would help with?

The Registry restricts only a single in-flight registerType action for a type, so this ordering ensures that we don't attempt to register both at once as that will fail.