Directory Param feature #2251

richbai90 · 2022-07-20T23:32:20Z

Notes for the reviewer

This is a follow up to #2167 it didn't make sense to continue on the same branch as the implementation is completely different.

Still not working: Ability to modify param values via command line

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

… we allow subsituting build variables in parameters. If we choose not, we can revert this easily.

…uired if we allow subsituting build variables in parameters. If we choose not, we can revert this easily." This reverts commit d66127b.

richbai90 · 2022-07-20T23:35:44Z

I did my best to follow the same coding practices I noticed in the existing source, but I can't guarantee I did everything the "right" way. If there is a better way to implement these features please provide advice.

… we allow subsituting build variables in parameters. If we choose not, we can revert this easily. Signed-off-by: Rich Baird <[email protected]>

…uired if we allow subsituting build variables in parameters. If we choose not, we can revert this easily." This reverts commit d66127b. Signed-off-by: Rich Baird <[email protected]>

…o its own function to facilitate additional growth. Signed-off-by: Rich Baird <[email protected]>

Signed-off-by: Rich Baird <[email protected]>

carolynvs

I didn't do a full review and am just evaluating the best way to represent directory parameters in a bundle, and where we can persist the data we need.

So let's figure those parts out first, and then we can move on to the part where you are stuck. I'm kind of hoping that this will get you unstuck. 🤞

carolynvs · 2022-07-25T20:50:19Z

pkg/config/config.go

@@ -56,6 +56,27 @@ const (
 	EnvPorterInstallationName = "PORTER_INSTALLATION_NAME"
 )

+type CustomParam struct {


I think a better place for this, where both pkg/manifest and pkg/cnab/config-adapter packages can reference it would be pkg/cnab. Hopefully that doesn't cause a circular dependency but otherwise we can find another place.

This config package is intended for Porter's config file and mapping it to command flags, etc.

carolynvs · 2022-07-25T21:07:14Z

pkg/config/config.go

+		Encoding: "base64",
+	},
+	"directory": {
+		Type: "string",


We will need to think a bit more about how we want a directory parameter type to be represented in the CNAB spec. Files as you see here, don't have a special representation, we just encode the contents to base64 and then do a bit of magic when we run the bundle to make the file on the other side.

I would love to see both of these types have a proper representation in CNAB eventually. At the moment we aren't actively changing the CNAB spec but let's see if we can pick something that is likely to just be accepted without major changes when we try to upstream these into the spec.

One way would be to have a well-defined "definition" entry in bundle.json (generated by adapter.go), that has a set of values, such as type: null, and $id to make it clear that it's a directory parameter.

bundle.json

{ "parameters": { "mydir": { "definition": "porter-directory-parameter", "description": "My directory contents", "destination": { "path": "/cnab/app/mydir" } }, "definitions": { "porter-directory-parameter": { "$id": "https://porter.sh/generated-bundle/#directory-parameter", "description": "A directory parameter", "type": "null" } } } }

Note: ALL parameters that are of type directory would use the same definition if we are going to use $id to identify directory parameters. If we use something else (like $comment), then we could do a different definition per parameter. I think id would be more straightforward?

carolynvs · 2022-07-25T21:13:17Z

pkg/cnab/directory_parameter.go

+// MountParameterSource represents a parameter using a docker mount
+// As a its source with the provided options
+type MountParameterSourceDefn struct {
+	mount.Mount `yaml:",inline"`


Let's talk more about this right here. The goal is that the bundle doesn't know or care how the directory got there. Just that it has the access it needs.

Ideally the mount options should all be a concern of just the porter client and how we execute the cnab bundle as a docker image. I may not have been clear in my original comment, but what I was suggesting was that the user could specify the mount options using a docker mount string, either as a --param flag, or via a parameter set:

# this is a parameter set file, not the porter.yaml name: mybundle-params parameters: - name: nginx-config source: # could specify whatever the docker --mount flag supports # target isn't required since Porter can determine that info from the parameter definition mount: type=bind,source=/home/myuser/myconfig,readonly

Then when the docker driver is used, it will use both the docker extension information in addition to the mount string from the parameter, to mount the directory into the container.

Okay I can use a string. My idea with using a larger object was so that we didn't have to parse the string, but it should otherwise simplify things a bit so I'll make the change.

carolynvs · 2022-07-25T22:29:01Z

pkg/cnab/provider/driver.go

@@ -44,6 +48,40 @@ func (r *Runtime) newDriver(driverName string, args ActionArguments) (driver.Dri
 		return nil, err
 	}

+	// Handle Directory support for the docker driver
+	// TODO: Handle directory support for other runtimes -- how?


Here's what I suggest:

Do not encode mount information into the directory parameter extension other than the information from DirectoryParameterDefinition. The rest is docker specific and should be specified by the user running the bundle, not the bundle author.

Check if docker is the driver, then handle the directory parameter just like you are now but... change where you get the information about the mount. Iterate over the bundle's parameters and set up mounts for each parameter of type directory.

Use the parameter's value as a docker --mount flag formatted string to get the mount details. The target will come from the parameter definition itself.

The extension data will need to provide details about each directory parameter. (will comment more about that on the data structures in question)

carolynvs · 2022-07-25T22:34:11Z

pkg/cnab/directory_parameter.go

+type DirectorySources struct {
+	Mount MountParameterSourceDefn `yaml:"mount,omitempty" json:"mount,omitempty"`
+}
+type DirectoryDetails struct {


I haven't tried this but here's what I think will fit better with how the CNAB spec and Porter works:

DirectoryDetails should have Name defined as well, or use a map[string]DirectoryDetails when storing the extension data and then key off of parameter name.

DirectoryDefaults should not have DirectorySources or Kind.

Do not create a new parameter source of type mount, I think we can get it to work without it and sources should be used for things that the bundle author knows about up-front (like using the output of a bundle as the parameter value), and bundle authors shouldn't be involved with whether or not a directory was injected using a docker mount, or through some other trick (like unpacking a gzip file into the specified directory).

… release/v1

richbai90 · 2022-08-04T07:50:57Z

One follow up, for the user and group feature, what should the effect of this setting be? Should the container run with that user id instead of the randomly generated one, or should it be for informational purposes to be handled by the mixin developer?

carolynvs · 2022-08-05T19:45:31Z

It won't affect the user that the bundle is run as. What I was thinking is that it should affect the owner and group for any files written by the mixin, because otherwise they will be owned by a wonky userid like 1001, when it probably needs to be owned by nginx or whatever is going to use the files written to the host.

richbai90 · 2022-08-09T16:11:50Z

What I was thinking is that it should affect the owner and group for any files written by the mixin

It seems to me that there are two ways to accomplish this.

Make this a documented contract that the mixin author needs to follow, but don't enforce it in code.
Create a preinstall step to scan the files in a directory, and a post install step to do the same. Compare the outputs and update the permissions of the files that have changed.
Is there another way I'm not considering? Of these options, which do you prefer?

carolynvs · 2022-08-10T13:46:40Z

Yeah... let's start with a documented contract that we expect mixin authors to follow but leave off any active enforcement. If we end up needing that, we can add it in later. I'd like to avoid putting something in that is too draconian and gets in the way of people doing what they want in their bundles.

carolynvs · 2022-10-21T15:10:23Z

@richbai90 Now that v1 is out, I was hoping we could collaborate on your PR/branch here and get this into an upcoming patch release. I see a few merge conflicts, let me know if you'd like help with them, I can resolve and push to your branch (if you gave maintainers push access in this PR).

Once that's fixed and we've had a chance to see how the tests are looking, we can figure out what the remaining work is. 👍

richbai90 · 2022-10-24T21:05:55Z

Hi Carolyn, I'm sorry for falling off the face of the earth. I'll give whomever access to the PR. Unfortunately, I'm no longer on the team where we were extensively working with this product and I would need time I don't currently have to get up to speed with V1 changes.

carolynvs · 2022-10-25T14:14:46Z

@richbai90 Not a problem and thank you for your time and effort on the feature design and implementation. I'll pick this up and make sure that the feature you worked on gets completed and shipped.

Rich Baird added 2 commits July 15, 2022 18:14

Make the bundle user details global to the build package. Required if…

d66127b

… we allow subsituting build variables in parameters. If we choose not, we can revert this easily.

Revert "Make the bundle user details global to the build package. Req…

d873618

…uired if we allow subsituting build variables in parameters. If we choose not, we can revert this easily." This reverts commit d66127b.

richbai90 requested review from carolynvs, VinozzZ and vdice as code owners July 20, 2022 23:32

richbai90 mentioned this pull request Jul 20, 2022

Advanced docker runtime #2167

Open

4 tasks

Rich Baird added 5 commits July 25, 2022 12:41

Make the bundle user details global to the build package. Required if…

a87e7de

… we allow subsituting build variables in parameters. If we choose not, we can revert this easily. Signed-off-by: Rich Baird <[email protected]>

Revert "Make the bundle user details global to the build package. Req…

a744f47

…uired if we allow subsituting build variables in parameters. If we choose not, we can revert this easily." This reverts commit d66127b. Signed-off-by: Rich Baird <[email protected]>

Create a new parameter type, and refactor parameter type checking int…

f26bba5

…o its own function to facilitate additional growth. Signed-off-by: Rich Baird <[email protected]>

add comments to the helper functions

ee52834

Signed-off-by: Rich Baird <[email protected]>

directory parameter added. TODO: Make customizable

b1b664e

Signed-off-by: Rich Baird <[email protected]>

richbai90 force-pushed the feature/directory_param branch from dd9877f to b1b664e Compare July 25, 2022 17:42

Fix the logic of IsDirSource to prevent duplicate mount errors

92e5499

Signed-off-by: Rich Baird <[email protected]>

carolynvs self-assigned this Jul 25, 2022

carolynvs reviewed Jul 25, 2022

View reviewed changes

Rich Baird added 5 commits July 26, 2022 16:02

Merge branch 'release/v1' of https://github.com/getporter/porter into…

52ab946

… release/v1

merge 1.0.0

d8448e2

make sure it builds

c04705e

merged parameters.go incorrectly, so fixed it

6099810

Cherry pick execute callback in closure

2a2ada8

carolynvs added the v1.1.0 label Sep 21, 2022

carolynvs added v1.2.0 and removed v1.1.0 labels Oct 21, 2022

carolynvs added the needs-followup Needs a maintainer to follow-up label Oct 25, 2022

kichristensen removed the v1.2.0 label Aug 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Directory Param feature #2251

Directory Param feature #2251

richbai90 commented Jul 20, 2022

richbai90 commented Jul 20, 2022

carolynvs left a comment

carolynvs Jul 25, 2022

carolynvs Jul 25, 2022

carolynvs Jul 25, 2022 •

edited

Loading

richbai90 Jul 27, 2022

carolynvs Jul 25, 2022

carolynvs Jul 25, 2022

richbai90 commented Aug 4, 2022

carolynvs commented Aug 5, 2022

richbai90 commented Aug 9, 2022

carolynvs commented Aug 10, 2022

carolynvs commented Oct 21, 2022

richbai90 commented Oct 24, 2022

carolynvs commented Oct 25, 2022

Directory Param feature #2251

Are you sure you want to change the base?

Directory Param feature #2251

Conversation

richbai90 commented Jul 20, 2022

Notes for the reviewer

Checklist

Reviewer Checklist

richbai90 commented Jul 20, 2022

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs Jul 25, 2022

Choose a reason for hiding this comment

carolynvs Jul 25, 2022

Choose a reason for hiding this comment

carolynvs Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

richbai90 Jul 27, 2022

Choose a reason for hiding this comment

carolynvs Jul 25, 2022

Choose a reason for hiding this comment

carolynvs Jul 25, 2022

Choose a reason for hiding this comment

richbai90 commented Aug 4, 2022

carolynvs commented Aug 5, 2022

richbai90 commented Aug 9, 2022

carolynvs commented Aug 10, 2022

carolynvs commented Oct 21, 2022

richbai90 commented Oct 24, 2022

carolynvs commented Oct 25, 2022

carolynvs Jul 25, 2022 •

edited

Loading