-
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
fix(manifest.go): fix Name property, add Description, add test #104
Conversation
Let's hold off on this for a sec until I can clear my head from vacation. I think there's a bit of a mixup between how porter resolves bundle dependencies, and how people expect it to that prompted this fix. I want to make sure that we have that clear in our heads before we merge any fixes related to that. |
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.
Just some rando comments. I'm glad you wrote a test. ❤️
I'm going to leave the rest to Jeremy since he wrote this.
@@ -80,3 +82,35 @@ func TestPorter_prepareDockerFilesystem(t *testing.T) { | |||
require.NoError(t, err) | |||
assert.True(t, execMixinExists, "The exec-runtime mixin wasn't copied into %s", wantExecMixin) | |||
} | |||
|
|||
func TestPorter_buildBundle(t *testing.T) { |
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.
Whatever else happens, let's make sure to keep a new test. 👍
@@ -20,7 +20,8 @@ type Manifest struct { | |||
path string | |||
outputs map[string]string | |||
|
|||
Name string `yaml:"image,omitempty"` |
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.
Looks like image is no longer being populated anymore?
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.
Indeed, if there was an image
field in the example porter.yaml at one point, it is no longer there. I've assumed this should point to the name
field, which is populated.
@carolynvs this PR originated with my using Later, upon reading the issue in #108, I theorized this user perhaps was experiencing the effects of the same issue described above. So, not so much around the larger issue(s) of how Porter resolves dependencies, but more just a simple bug fix to ensure the go struct ( |
Thanks for clarifying! I'm going to let @jeremyrickard take this from here, since it's not dependency related then. |
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.
LGTM
The main change here (mapping
Manifest.Name
toname
in yaml, as opposed toimage
), fixes the issue wherein thename
field in thebundle.json
would be rendered empty after aporter build
. This empty field prevents successful installation of the bundle (it cannot be found.)