-
Notifications
You must be signed in to change notification settings - Fork 201
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
Enable setting custom values during porter build. #1900
Enable setting custom values during porter build. #1900
Conversation
16041b1
to
864c2d5
Compare
There are probably still various things that I am not sure of ranging from the (1) variable naming on several places, (2) the duplicate of I hope this is getting nearer to what the issue is asking for. Looking forward to the review, @carolynvs ! 🙂 |
Currently this PR is only able to cater for The type issue on the templating language is addressed separately on this issue. |
I wholeheartedly approve of demonstrating functionality through whalesay 🐳 |
pkg/build/docker/docker.go
Outdated
@@ -30,6 +31,20 @@ func NewBuilder(cxt *portercontext.Context) *Builder { | |||
} | |||
|
|||
func (b *Builder) BuildInvocationImage(ctx context.Context, manifest *manifest.Manifest) error { | |||
buildArgs := make(map[string]*string) |
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.
Since we have this same logic in a couple places, how about we put this in a new package, pkg/build/driverutil, and then call it something like driverutil.GetBuildArgs
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.
The docker one expects a map[string]*string
for its image's buildArgs while buildkit expects a map[string]string
. can we achieve this with different expected output type? Should we use the build driver as a parameter and let the function determine the type of the map value from it? Example as follows.
func GetBuildArgs(custom manifest.CustomDefinitions, buildDriver string) (interface{}, error) {
switch buildDriver {
case "docker":
output := make(map[string[*string)
// flatten customDefinitions and put it in the map
return output, nil
case "buildkit"
output := make(map[string[string)
// flatten customDefinitions and put it in the map
return output, nil
default:
return nil, errors.New("build driver is not expected: %+v", buildDriver)
}
}
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.
What you have in the PR now is fine 👍
7fe1dea
to
b7debe3
Compare
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.
Everything looks great but we still need documentation for the website explaining your new feature so that people can use it.
Let's update the https://release-v1.porter.sh/author-bundles/#custom section and show people how to build a bundle and set custom metadata.
pkg/build/driverutil/driverutil.go
Outdated
for innerKey, innerValue := range v { | ||
out[key+"."+innerKey] = innerValue | ||
} | ||
default: |
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.
What will happen if someone has custom values like this? It seems like it would hit the default and then return an error. If we don't want to implement more cases in this PR, let's make sure the documentation is explicit about what will work (only string values) and then create an issue to track supporting other types such as bool, number and arrays.
custom:
tags:
- blue
- green
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.
Ah, sorry that I totally missed this possibility of array type! Thank you for pointing this one out.
Previously you mentioned we can use the example of --param
flag on install
command on retrieving the flag's values, which I used in this one using ParseVariableAssignments
like how it was used on this one for the install
's param
. But when I tried to supply the value using comma separated one e.g. key="value1,value2"
or key=value1,value2
, it yield the error of invalid parameter (%s), must be in name=value format
from this particular line, like my screenshot below.
Would it be okay to add handling case of multiple comma separated values for this ParseVariableAssignments
method as well (which will still be used by install
's --param
)?
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.
The way StringSliceVar
works in cobra (which is how you defined the flag) is to allow the user to specify the parameter multiple times. Like so, "--param A=1 --param B=2", then cobra binds that to the slice like so []string{"A=1", "B=2"}
. So there's no need to do further parsing work to support it.
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.
So what I was asking wasn't about an array containing different pairs of key-value like you mentioned e.g. (--param A=1,B=2
since this can be done using --param A=1 --param B=2
like you've shown), but more of a one specific particular key that has array value (--param A=1,2
), since I assume it would be needed to support the array value of custom properties like you mentioned earlier.
custom:
version: 0.1
tags:
- blue
- green
(I assume this will need --custom version=0.2 --custom tags=red,yellow
, referring to the tags
key).
I tried to log the output and StringSliceVar
automatically split using comma as the delimiter instead, so it reads the next value which should be together as array value as its own (the b
and c
) and thus resulted on error from this particular part.
While I tried using StringArrayVar
but it automatically resulted in a panic.
I haven't found any workaround to support the array value for this custom
property so if it would be okay I would like to just support string
key-value pair for this iteration to at least be able to be used by people that need this use case (like I mentioned on the 2nd point of my recent comment).
pkg/build/docker/docker.go
Outdated
@@ -30,6 +31,20 @@ func NewBuilder(cxt *portercontext.Context) *Builder { | |||
} | |||
|
|||
func (b *Builder) BuildInvocationImage(ctx context.Context, manifest *manifest.Manifest) error { | |||
buildArgs := make(map[string]*string) |
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.
What you have in the PR now is fine 👍
Hi @carolynvs , truly sorry that I am just continuing upon the particular PR after a really long time. If I may I've got some questions while I'm working through untangling the merge conflict.
will be
|
Yup, --build-arg and --custom have overlap but I decided it was worth it because in one case someone isn't thinking about the porter bundle itself and is just trying to use docker features that they know (--build-arg) to parameterize their dockerfile. While as you said, custom is intended to help you use that value in your bundle as well. I'm glad you asked about this because when both --custom and --build-arg have the same key, e.g. --custom MYKEY=1 --build-arg MYKEY=2, we should have the --custom flag take precedence so that the build arg and the bundle.custom have the same value when used in porter.yaml. Yeah, per #1789 (comment) lets just do strings for now. |
d2d8ebd
to
ee6b09c
Compare
@carolynvs I've fixed the file conflicts and added the documentation on https://release-v1.porter.sh/author-bundles/#custom . I'll file the issue to track for other data type after this PR got successfully merged. Again, I hope you're not tired hearing me saying thank you since I definitely wouldn't be able to finish any without your tremendous help and enormous patience. Thank you, Carolyn! |
cmd/porter/bundle.go
Outdated
@@ -87,6 +88,9 @@ func buildBundleBuildCommand(p *porter.Porter) *cobra.Command { | |||
"Secret file to expose to the build (format: id=mysecret,src=/local/secret). May be specified multiple times.") | |||
f.BoolVar(&opts.NoCache, "no-cache", false, | |||
"Do not use the Docker cache when building the bundle's invocation image.") | |||
f.StringSliceVar(&opts.Customs, "custom", nil, |
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.
I've recently learned that we are using the wrong function here when defining parameters that may be repeated. The function StringSliceVar supports the following functionality --custom VAR1=A,VAR2=B
which is equivalent to --custom VAR1=A --custom VAR2=B
. The cobra library splits the flag value based on any commas present (and prevents people from specifying values that have commas!)
We should be using f.StringArrayVar
instead, which doesn't try to parse based on commas.
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.
Got it, I assume it was similar to this one #1917 (comment) referring to --param
flag on install command that is addressed on #1931.
@@ -511,6 +512,12 @@ You can access custom data at runtime using the `bundle.custom.KEY.SUBKEY` templ | |||
For example, `{{ bundle.custom.more-custom-config.enabled}}` allows you to | |||
access nested values from the custom section. | |||
|
|||
Multiple custom values that were defined at the manifest can also be injected with new values during build time using the \--custom values tied to the `porter build` command. Currently only support string value. You can use dot notation to specify a nested field: |
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.
Multiple custom values that were defined at the manifest can also be injected with new values during build time using the \--custom values tied to the `porter build` command. Currently only support string value. You can use dot notation to specify a nested field: | |
Multiple custom values that were defined in the manifest can also be injected with new values during build time using the \--custom values tied to the `porter build` command. Currently only supports string values. You can use dot notation to specify a nested field: |
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
…ter build Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
… build process Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
f73007a
to
f8be9a9
Compare
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.
I'm looking forward to having this functionality in porter finally! 🎉
What does this change
Enable user to use
--custom
flag for setting custom values when building a particular bundle usingporter build
command.porter-1789-custom-values-porter-build-08-02.mp4
What issue does it fix
#1789
Notes for the reviewer
Put any questions or notes for the reviewer here.
Checklist
Reviewer Checklist