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

Add ExtendedBundle wrapper #1739

Merged
merged 3 commits into from
Aug 26, 2021

Conversation

carolynvs
Copy link
Member

What does this change

This wraps the cnab-go bundle and adds support for custom extensions that porter supports. It will help us wean off of using the porter manifest at runtime, which is a huge problem because then our support for spec extensions only works when it was authored in Porter.

More work is needed to fix this, but this is just a refactoring to get the data in the right place, i.e. when we do pass around a bundle, it's a bundle that knows how to deal with extensions.

I am still saving a cnab bundle document so that if we change the structure of the wrapper it doesn't impact the stored document.

What issue does it fix

Chipping away at the root cause of #1024. We need to get to a place where the porter.yaml is only used to generate the bundle, and to pass off actions/config to the mixins. We shouldn't be using it in the porter runtime (where we decide if file parameters should be supported for example, or parse dependencies), and should be acting off the bundle.json entirely.

Notes for the reviewer

Sorry for all the refactoring lately! 😬

Checklist

  • Unit Tests
  • Documentation
  • Schema (porter.yaml)

@carolynvs carolynvs force-pushed the extended-bundle-wrapper branch from 4c3ad07 to fb58b9f Compare August 25, 2021 20:49
@@ -303,6 +303,9 @@ func validateCredentialName(args []string) error {

func (p *Porter) CredentialsApply(o ApplyOptions) error {
namespace, err := p.getNamespaceFromFile(o)
if err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bonus bug fix! 🤦‍♀️

@carolynvs carolynvs force-pushed the extended-bundle-wrapper branch from fb58b9f to dd46a93 Compare August 25, 2021 21:00
@carolynvs carolynvs marked this pull request as ready for review August 25, 2021 21:00
@carolynvs
Copy link
Member Author

/azp run porter-integration

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This wraps the cnab-go bundle and adds support for custom extensions
that porter supports. It will help us wean off of using the porter
manifest at runtime, which is a huge problem because then our support
for spec extensions only works when it was authored in Porter.

More work is needed to fix this, but this is just a refactoring to get
the data in the right place, i.e. when we do pass around a bundle, it's
a bundle that knows how to deal with extensions.

I am still saving a cnab bundle document so that if we change the
structure of the wrapper it doesn't impact the stored document.

Signed-off-by: Carolyn Van Slyck <[email protected]>
Signed-off-by: Carolyn Van Slyck <[email protected]>
Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

}

// DependencyReader is a Reader for the DependenciesExtension, which reads
// from the applicable section in the provided bundle and returns a the raw
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// from the applicable section in the provided bundle and returns a the raw
// from the applicable section in the provided bundle and returns the raw

Looks like there are other spots where this typo exists, in other files. To the search and replace!

}

// FileParameterReader is a Reader for the FileParameterExtension.
// The extension does not have any data, it's presence indicates that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// The extension does not have any data, it's presence indicates that
// The extension does not have any data, its presence indicates that

Signed-off-by: Carolyn Van Slyck <[email protected]>
@carolynvs carolynvs merged commit a150d84 into getporter:release/v1 Aug 26, 2021
@carolynvs carolynvs deleted the extended-bundle-wrapper branch August 26, 2021 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants