-
Notifications
You must be signed in to change notification settings - Fork 208
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
Define a bundle with the v2 dependencies extension #2323
Define a bundle with the v2 dependencies extension #2323
Conversation
dcbc21d
to
c79bd15
Compare
fdcffa6
to
cc0967b
Compare
cc0967b
to
242ca7e
Compare
d7ef3b2
to
075e51f
Compare
e95b6ee
to
e864c75
Compare
"db": { | ||
"name": "db", | ||
"bundle": "localhost:5000/mydb:v0.1.0", | ||
"parameters": { |
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 aren't adding a new version of parameter sources to support wiring passing a parameter from the root bundle to a child bundle. It may seem like that would work but remember that parameter sources can only reference what is known at bundle definition time, and bundles don't know that they are embedded as dependencies.
They can't "pull" the value they need by defining it in a parameter source, they wouldn't have enough information to say "give me the parameter named X that was given to my parent and map it to my parameter Y". Instead we are defining a way to "push" the value from the root down to the dependent bundle, since the parent bundle does know the name of its own parameters and those of its dependencies.
I am open to naming "parameters" here something different because it's not immediately clear based on the name that this is mapping from the parameters of the current bundle to the parameters defined on its dependencies.
84891b9
to
ad6f58b
Compare
9d12370
to
aca2818
Compare
0092e75
to
3ab43a9
Compare
Ok this is finally ready for review, apologies for the mega PR! |
b407a79
to
d6d0bc4
Compare
4f06f67
to
91c493a
Compare
This adds support for the dependencies-v2 experimental flag when building a bundle. * Add dependency v2 fields to the porter.yaml manifest * Represent v2 deps in the bundle.json * Determine which dependency extension is used by a bundle * Have separate packages for the dependencies v2 cnab extension, and its implementation * Updated porter schema command to output the experimental bundle schema when the dependencies-v2 flag is set I have updated the mybuns test bundle with additional data so that it can be used in some of our unit tests (such as the cnab-adapter tests) so that we aren't maintaining a bunch of random test porter.yaml files that are all slightly different to hit all our required test cases. I'll follow up later and see if we can reuse it even more elsewhere. While not an officially released version of the manifest, I have reserved 1.1.0 for bundles that define advanced dependencies. The schemaVersion of a bundle can only be set to 1.1.0 when the v2 dependencies experimental feature is enabled. The docs still point to 1.0.1 as the latest version, and by default bundles are created with 1.0.1 and validated against 1.0.1. Once we are sure that our schema changes are solid and won't be modified further under the experimental flag (ideally waiting until the flag is removed) then we can release 1.1.0 and default to it. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
5c0e42a
to
d9f9659
Compare
This is is pretty big and will probably need to have more of a deep dive but from a cursory glance it looks good! A few comments: I wanted to be able to pass the outputs of 1 dependency to either the parameter or credential of another dependency and it looks like that case is covered. Just a clarification if the output of the dependency bundle is marked as sensitive then the resolving of passing it into the second dependency will result in a lookup of that secret? Also I think this is a more advanced uses case that might not even be totally applicable but I wanted to clarify that it is NOT meant to be supported yet. What if I wanted to bookend some dependency bundles inside of a single porter bundle. i.e. I want to run say a portion of my "install" stage to setup something, then run a dependency, then run more install steps. Idk if that will ever be necessary as a supported use case but the example I'm thinking of is say we need an azure sql database but the bundle that creates it requires a resource group as an input. The proper solution is probably to have another dependency that creates that resource group and then feed that into the sql bundle dependency but would there ever be a use case where you could handle some "init" tasks inside your bundle before running any dependency? |
Yes, you can use the standard porter template syntax that you are used to seeing when writing a bundle action, such as: dependencies:
requires:
- name: mysql
- name: myapp
parameters:
connstr: ${bundle.dependencies.mysql.outputs.admin-connstr} I am planning on supporting full template syntax, including concatenation so that eventually it can even support combining multiple variables for example:
Porter will continue to resolve and inject values (such as parameter sets, credential sets, or sensitive outputs from another bundle) just-in-time. We will validate that everything can be resolved when we start running the root bundle, and then right before any dependency is run, we resolve and inject values into the bundle. We do not temporarily persist values or keep around stale data that could have changed.
There is no plan to change when dependencies are run, they are always executed before the parent bundle runs (though uninstall is executed in reverse dependency order). However, as part of "mixins as bundles" feature (PE005), you will be able to call an other bundle as a step in your own bundle and pass data to it just like you would a mixin today. # this is made up syntax, the feature is not designed
install:
- exec:
command: ./init.sh
- bundle:
reference: docker.io/my/bundle:v1.2.3
parameters:
logLevel: 11
credentials:
token: ${bundle.credentials.github-token}
outputs:
NAME_OF_BUNDLE_OUTPUT: NAME_OF_DEPENDENCIES_OUTPUT # will support template syntax too In addition, you can also define a workflow (this is part of PEP003) and can execute bundles that way instead of embedding a call to a bundle inside of another bundle. This is useful to execute a series of bundles, passing data between them, even when there isn't a dependency or parent/child relationship. You can just pick arbitrary bundles and actions and then porter will run them. |
) | ||
|
||
// DependenciesV2Extension represents the required extension to enable dependencies | ||
var DependenciesV2Extension = RequiredExtension{ |
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.
nit: Definitely not in love with the naming of these structures/methods having "v2" directly in them. Would it be worthwhile to create a dependencies interface and use more like the factory pattern?
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 would involve a larger change to how we define and process CNAB extensions which is out of scope for this issue. I am not certain that I see a way for it to work since there isn't a consistent interface for extensions, they return a unique data structure representing the parsed extension data from the custom section of bundle.json. So the docker extension returns a different parsed structure than the v1 deps extension, than the v2 deps extension, etc.
The use of the extension you see defined here is limited to this package. Elsewhere when you want to read out the dependencies v2 data structure from the bundle, you can do this:
var bundle cnab.ExtendedBundle // initialize this somehow
if bundle.HasDependenciesV2() {
depsDef := bundle.ReadDependenciesV2()
// ...
}
If you want to take a stab at it anyway, I recommend a follow-up draft PR showing how you'd like to see it changed since I'm just not seeing it since I'm a bit too used to how it's implemented today.
What does this change
This adds support for the dependencies-v2 experimental flag when building a bundle.
I have updated the mybuns test bundle with additional data so that it can be used in some of our unit tests (such as the cnab-adapter tests) so that we aren't maintaining a bunch of random test porter.yaml files that are all slightly different to hit all our required test cases. I'll follow up later and see if we can reuse it even more elsewhere.
While not an officially released version of the manifest, I have reserved 1.1.0 for bundles that define advanced dependencies. The schemaVersion of a bundle can only be set to 1.1.0 when the v2 dependencies experimental feature is enabled.
The docs still point to 1.0.1 as the latest version, and by default bundles are created with 1.0.1 and validated against 1.0.1. Once we are sure that our schema changes are solid and won't be modified further under the experimental flag (ideally waiting until the flag is removed) then we can release 1.1.0 and default to it.
What issue does it fix
Closes #2225
Notes for the reviewer
For now we will define our custom v2 implementation of dependencies under the extension
org.getporter.dependencies@v2
and if the draft is accepted, we will move to the final extension nameio.cnab.dependencies@v2
(while continuing to support the old extension name).If there are changes to the proposed spec, we can decide to support v2 of porter's dependencies and then implement v2 of the cnab spec and support both.
Checklist
Reviewer Checklist