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 ImageMap #419

Merged
merged 2 commits into from
Jun 19, 2019
Merged

Add ImageMap #419

merged 2 commits into from
Jun 19, 2019

Conversation

carolynvs
Copy link
Member

@carolynvs carolynvs commented Jun 14, 2019

This adds manual support for image map. The data can be defined in the porter.yaml and porter will populate it in the generated bundle.json file, so that it is made available at runtime per the spec.

However porter and the mixins do not have any explicit behaviors based on the presence of the data. When a user populates the image map, they must also add scripts to the bundle to use it at runtime.

@carolynvs carolynvs added this to the CNAB 1.0 milestone Jun 17, 2019
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. Only a few questions/thoughts; no requested changes per se.

pkg/porter/build.go Show resolved Hide resolved
pkg/porter/build.go Show resolved Hide resolved
@@ -233,3 +233,46 @@ func TestPorter_paramRequired(t *testing.T) {
require.True(t, ok)
require.True(t, p2.Required)
}

func TestPorter_generateImages(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want test(s) that vet the required fields when defining an ImageMap in a porter.yaml vs fields that can be empty (omitempty)? Or would this more be testing general yaml -> golang struct flow and thus perhaps not needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I'm not quite sure what you want to test here? Do you mean like the kind of test that I did for cnab-go here? To ensure that the proper set of fields have omit empty and force people to think about it each time a field is added?

cnabio/cnab-go#24

Copy link
Member

Choose a reason for hiding this comment

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

Oh yup, something similar to what you've done in the link provided. Can be a follow-up if we deem it necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I can do that in a follow-up PR since it's a bit larger in scope than just image map, and we can adjust which ones do have omit empty on them at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure I'm going to get to this today so here's an issue to track it #425

@carolynvs carolynvs merged commit 96006a5 into getporter:master Jun 19, 2019
@carolynvs carolynvs deleted the doom-images branch June 19, 2019 15:56
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.

3 participants