Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
rfc: Cloud Assembly Specification #1119
rfc: Cloud Assembly Specification #1119
Changes from 1 commit
a43db28
4325c6f
ed7e6a5
0ceb93c
36f8998
27bd29b
af266bc
7878c51
c033016
d923d68
b619cbd
f43e192
8f05c1d
55714ee
9e49de4
03ad5ae
606662c
173764f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be "tenets" (or "design goals"). To write good tenets, a nice trick I've learned is to think (and even describe) what it is NOT. The purpose of tenets is to help make design decisions. Each tenet should describe a dimension or continuum and state which side of the continuum we are going to prefer.
Also, there's a bit of conflation here between goals for the spec it self (e.g. "cloud agnostic", there's no goal for assemblies to be cloud agnostic) and the assembly (e.g. "extensible", there isn't a goal for the spec to be extensible).
Anyway - this needs a bit more work, happy to spend some face to face time on this if you like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I very much would like the specification to be fundamentally cloud-agnostic, which is why I use "the cloud" and not "AWS". As for extensibility, this may not be the right term indeed (settled for this now until I figure out a better one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
index.json
? Give it a bit of a "web standard" oraThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"If it's a cat, call it a cat".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should include a JSON schema and/or a typescript type definition (preferably the former generated from the latter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that was always the plan :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec is missing a section on how to publish new drop types. Specifically, it should include a JSON schema which allows tools to validate the manifest "properties" section against
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a part of it but I reckon there might be more to it than just this. It'll be pretty inefficient to discuss over GitHub issues, though. Let's talk about this & report the outcome back here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are the escape sequences valid? All strings everywhere in the cloud assembly? Keys, values, both? Would be good to clarify I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. They're only meaningful in places where the interpolation is (aka - values of the
properties
map).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks nice, but I am wondering if it's at all possible to implement a deployment tool that can interact with drop types from multiple languages.
Perhaps the drop type provider is not a class but rather a POSIX program and the protocol between the deployment tool and the provider is based on STDIN/STDOUT/ARGV/ENV.... They will be discovered by a PATH look-up (and will include some version handshake). Might be a much more powerful model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I was thinking but I wanted to talk about it before writing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these are tying declaration to implementation.
More generically, how about just an identifier string? (Could be namespaced a la
com.amazonaws.cloudformationstack
or something)In that case, we can even build tools to statically analyze the drops, without necessarily executing them. In direct opposition is the following:
How is any tool going to analyze all the CloudFormation stacks in an Assembly if this is the format of "identifier" they may have to contend with?
A compromise might be found in the following:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of the
URI
style string is that the protocol string allows you to specify a mechanism for accessing (e.g:npm://
means you would neednode
to be available & get the support by runningnpm install <packageName>
). Then the path segment would give info about the package coordinates, and then how to use it. Implementations would support certain protocols and know how to execute. So nobash -c blah
here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was responding to Elad's suggestion of using subprocesses here.
Even if we were to use
npm://
coordinates, does the identifier have meaning without the NPM package that contains code to interpret it? To have any hope of tools that interpret the model apart from simply executing it, it must.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree with Rico. Why limit ourselves to a string, when we have the full power of JSON available? I think the
Drop Type
being an object is more powerful (and easier to extend in the future).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already seems not powerful enough (cross-region CodePipelines? Cross-account CodePipelines?).
Again, I would suggest using more of JSON's power here than just a
string
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A
Drop
lives in exactly one account/region. Cross-region / cross-account CodePipelines will involve multipleDrop
s in different environments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stackName
needs to be a parameter as well.Or will you force
stackName
== logicalID ofdrop
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think I was implicitly assuming logicalId = stackName, but that feels brittle, especially if you want to computer-generate the logical ID's (the spec, after all, doesn't bind a particular meaning to them beyond "they identify a thing with the doc").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the only way to order drops by using the
output
of one in theparameters
of another? We have more need of sequencing stack drops that aren't necessarily expressed as a threading of information from one object to the other (yay, side effects).For example, an
Export
in one stack and anImport
in the other.I would argue for a field similar to CloudFormation's
DependsOn
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Export/Import is a good case. Although one could say you might want to use Parameters to make the dependency explicit (but you certainly don't have to). I'm adding an optional
dependsOn
array.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, taking the output here would purely be to force sequencing of operations, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. There is also an option where we may not get a bucketName input, and instead the provisioning tool figures it out for us (so the parameter might become optional). I'm not entirely sure yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the
helperStack
is the bootstrap stack? Does a CDK app always add in the bootstrap stack into the assembly, or is the bootstrap stack supplied as a parameter to the assembly?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is the case where you explicitly specify "everything" in your cloud assembly, but in case your "provisioning orchestrator" does this for you, then it wouldn't be part of the assembly. That's where the optional inputs I referred to in a previous reply come into play - the provisioning orchestrator wires it's own values in at deploy time, and they are available for reading from the asset
Drop
's outputs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer for this name to be something like
pushTarget
or something.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah these serve mostly as example, so names in here are definitely non-normative and I've been... favoring speed over quality :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this name different from input
imageName
so that it's clear it's not the same schema.fullImageName
?qualifiedImageName
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would
imageId
make sense here? I guess it collides unfortunately with docker terminology...