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

Define a bundle with the v2 dependencies extension #2323

Merged
merged 2 commits into from
Apr 10, 2023

Conversation

carolynvs
Copy link
Member

@carolynvs carolynvs commented Sep 1, 2022

What does this change

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.

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

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

@carolynvs carolynvs added pep003-advanced-dependencies Implementation of the Advanced Dependencies proposal playground 🙈 Pull request to try stuff out and see if it builds. Don't look! labels Sep 1, 2022
@getporterbot getporterbot added this to In Progress in Porter and Mixins Sep 1, 2022
@carolynvs carolynvs changed the title Depsv2 cnab representation Define a bundle with the v2 dependencies extension Sep 1, 2022
@carolynvs carolynvs changed the base branch from release/v1 to main October 25, 2022 18:13
@carolynvs carolynvs force-pushed the depsv2-cnab-representation branch 2 times, most recently from fdcffa6 to cc0967b Compare October 25, 2022 20:52
@carolynvs carolynvs removed the playground 🙈 Pull request to try stuff out and see if it builds. Don't look! label Oct 25, 2022
@carolynvs carolynvs force-pushed the depsv2-cnab-representation branch 3 times, most recently from d7ef3b2 to 075e51f Compare November 15, 2022 17:01
@carolynvs carolynvs force-pushed the depsv2-cnab-representation branch 2 times, most recently from e95b6ee to e864c75 Compare December 23, 2022 22:35
"db": {
"name": "db",
"bundle": "localhost:5000/mydb:v0.1.0",
"parameters": {
Copy link
Member Author

@carolynvs carolynvs Feb 1, 2023

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.

@carolynvs carolynvs force-pushed the depsv2-cnab-representation branch 3 times, most recently from 84891b9 to ad6f58b Compare February 9, 2023 23:15
@carolynvs carolynvs force-pushed the depsv2-cnab-representation branch 2 times, most recently from 9d12370 to aca2818 Compare February 16, 2023 18:43
@carolynvs carolynvs force-pushed the depsv2-cnab-representation branch 4 times, most recently from 0092e75 to 3ab43a9 Compare February 28, 2023 18:37
@carolynvs carolynvs marked this pull request as ready for review March 2, 2023 16:48
@carolynvs carolynvs requested a review from vdice March 2, 2023 16:48
@carolynvs
Copy link
Member Author

Ok this is finally ready for review, apologies for the mega PR!

@carolynvs carolynvs force-pushed the depsv2-cnab-representation branch 2 times, most recently from b407a79 to d6d0bc4 Compare March 28, 2023 16:44
@carolynvs carolynvs marked this pull request as draft March 29, 2023 16:05
@carolynvs carolynvs marked this pull request as ready for review March 29, 2023 21:49
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 <[email protected]>
@sgettys
Copy link
Contributor

sgettys commented Mar 30, 2023

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?

@carolynvs
Copy link
Member Author

carolynvs commented Mar 30, 2023

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.

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:

https://${bundle.parameters.host}:${bundle.dependenices.mysql.outputs.port}/api/v1

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?

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.

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?

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{
Copy link
Contributor

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?

Copy link
Member Author

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.

@carolynvs carolynvs merged commit 28ee71e into getporter:main Apr 10, 2023
Porter and Mixins automation moved this from In Progress to Done Apr 10, 2023
@carolynvs carolynvs deleted the depsv2-cnab-representation branch April 10, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pep003-advanced-dependencies Implementation of the Advanced Dependencies proposal
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Define advanced dependencies in a bundle
2 participants