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

Enable setting custom values during porter build. #1900

Merged

Conversation

joshuabezaleel
Copy link
Contributor

@joshuabezaleel joshuabezaleel commented Feb 8, 2022

What does this change

Enable user to use --custom flag for setting custom values when building a particular bundle using porter 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

  • 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

@joshuabezaleel
Copy link
Contributor Author

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 convertMap and its test and whether we can write it only once for the buildkit and docker, and (3) whether the new test case already cover the scenario on the build_integration_test.go and generateManifest_test.go, but I guess I plan to put this Pull Request the earliest I can be as to gather feedback and inputs as soon as possible.

I hope this is getting nearer to what the issue is asking for. Looking forward to the review, @carolynvs ! 🙂

@joshuabezaleel
Copy link
Contributor Author

Currently this PR is only able to cater for string type value on the porter.yaml manifest. This is because since our templating language needs encapsulating double quote to instantiate the variable value for example "{{ bundle.custom.customKey1 }}" which the value will always be surrounded by double quote e.g. "editedCustomValue1" no matter whether we put double quote (to denote string) or not (to denote boolean or integer) when we pass the custom value flag on porter build command.

The type issue on the templating language is addressed separately on this issue.

@carolynvs
Copy link
Member

I wholeheartedly approve of demonstrating functionality through whalesay 🐳

cmd/porter/bundle.go Outdated Show resolved Hide resolved
pkg/build/buildkit/buildx.go Outdated Show resolved Hide resolved
@@ -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)
Copy link
Member

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

Copy link
Contributor Author

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)
    }
}

Copy link
Member

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 👍

pkg/porter/generateManifest.go Outdated Show resolved Hide resolved
pkg/porter/build_integration_test.go Outdated Show resolved Hide resolved
@carolynvs carolynvs self-assigned this Feb 20, 2022
@joshuabezaleel joshuabezaleel force-pushed the 1789-custom-values-porter-build branch 2 times, most recently from 7fe1dea to b7debe3 Compare February 22, 2022 04:15
Copy link
Member

@carolynvs carolynvs left a 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.

for innerKey, innerValue := range v {
out[key+"."+innerKey] = innerValue
}
default:
Copy link
Member

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

Copy link
Contributor Author

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.
image

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)?

Copy link
Member

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.

Copy link
Contributor Author

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.
image

While I tried using StringArrayVar but it automatically resulted in a panic.
image

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).

@@ -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)
Copy link
Member

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 👍

@joshuabezaleel
Copy link
Contributor Author

joshuabezaleel commented Apr 20, 2022

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.

  1. I saw on the recent PR of yours that added various features for the Docker support (it is a really good one btw, hat tip! 🎉) that you also add the --build-arg flag support for the porter build command. Since both the --build-arg and --custom flag on the command will essentially be build arguments for the Docker image, how will the usage these two differ? When should people use --custom and not just --build-arg? I am assuming the difference is just that for the custom key-value pairs it will (1) be defined on the porter.yaml manifest and also (2) have that default value defined on the manifest as well?

  2. Would it be okay if this currently only support for just the string type? For boolean and integer value I assume it would need to wait for this particular issue of templating language to be able to be supported. While for the array, I have a bit of confusion since BuildArgs for the image is expecting a string value (from map[string]string). If we are going to support array, does this mean that it will be a string of comma-separated value? Example as follows:

custom:
  tags: 
    - blue
    - green

will be

ARG CUSTOM_TAG=blue,green

@carolynvs
Copy link
Member

carolynvs commented Apr 21, 2022

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.

@joshuabezaleel joshuabezaleel force-pushed the 1789-custom-values-porter-build branch 4 times, most recently from d2d8ebd to ee6b09c Compare April 27, 2022 11:20
@joshuabezaleel
Copy link
Contributor Author

@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!

@@ -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,
Copy link
Member

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.

Copy link
Contributor Author

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.

cmd/porter/bundle.go Show resolved Hide resolved
@@ -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:
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
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>
@joshuabezaleel joshuabezaleel force-pushed the 1789-custom-values-porter-build branch from f73007a to f8be9a9 Compare April 27, 2022 16:23
Copy link
Member

@carolynvs carolynvs left a 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! 🎉

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