-
Notifications
You must be signed in to change notification settings - Fork 214
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
Use user specified directory for resolving file path #2142
Conversation
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
cb4aef5
to
e2efc53
Compare
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
e2efc53
to
8b92c4d
Compare
@VinozzZ When you are ready for a review, just move it out of draft 👍 |
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.
This is looking good! I just have some small suggestions
cmd/porter/bundle.go
Outdated
@@ -80,7 +80,7 @@ The docker driver builds the bundle image using the local Docker host. To use a | |||
f.StringVar(&opts.Name, "name", "", "Override the bundle name") | |||
f.StringVar(&opts.Version, "version", "", "Override the bundle version") | |||
f.StringVarP(&opts.File, "file", "f", "", | |||
"Path to the Porter manifest. Defaults to `porter.yaml` in the current directory.") | |||
"Path to the Porter manifest. When build context directory is specified, relative path will be resolved using the build context directory as the root directory. Defaults to `porter.yaml` in the current directory.") |
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 think we can simplify this a bit since the build context directory defaults to the working directory.
"Path to the Porter manifest. When build context directory is specified, relative path will be resolved using the build context directory as the root directory. Defaults to `porter.yaml` in the current directory.") | |
"Path to the Porter manifest. The path is relative to the build context directory. Defaults to porter.yaml in the current directory.") |
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 wanted to make sure that users understand that if a absolute path is passed in, then it won't be relative to the build context directory. How about we say
`When a relative path is provided, the path is relative to the build context directory"
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 build context directory (--dir) defaults to the current directory. So when you say "relatative to the build context directory" I think that covers both cases, no?
I just double checked and --dir doesn't say "Defaults to the current directory". How about we add that (it should have been included so that's an oversight)? I'm trying to avoid complex explanations, and explaining the --dir flag on the helptext for the --file flag.
pkg/porter/cnab.go
Outdated
if o.Dir != "" && !filepath.IsAbs(o.File) { | ||
o.File = cxt.FileSystem.Abs(filepath.Join(o.Dir, o.File)) |
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.
Isn't o.Dir always set in the if statement above? I think we can simplify this to the following
if o.Dir != "" && !filepath.IsAbs(o.File) { | |
o.File = cxt.FileSystem.Abs(filepath.Join(o.Dir, o.File)) | |
if !filepath.IsAbs(o.File) { | |
o.File = cxt.FileSystem.Abs(filepath.Join(o.Dir, o.File)) |
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 thought the o.Dir
is only resolved if it's not empty in the if statement above?
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 okay thanks I thought that if it was empty, we were setting it to the absolute path of the current working directory. If we did that then we wouldn't need a bunch of defensive checks later in the code since we know it was initialized properly. What do you think?
pkg/porter/cnab.go
Outdated
@@ -164,13 +168,17 @@ func (o *bundleFileOptions) defaultBundleFiles(cxt *portercontext.Context) error | |||
} else if o.CNABFile != "" { // --cnab-file | |||
// Nothing to default | |||
} else { | |||
manifestExists, err := cxt.FileSystem.Exists(config.Name) | |||
defaultPath := config.Name | |||
if o.Dir != "" { |
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.
Again, isn't o.Dir always set?
pkg/porter/cnab.go
Outdated
if err != nil { | ||
return errors.Wrap(err, "could not check if porter manifest exists in current directory") | ||
} |
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.
if err != nil { | |
return errors.Wrap(err, "could not check if porter manifest exists in current directory") | |
} | |
if err != nil { | |
return errors.Wrapf(err, "could not find a porter manifest at %s", defaultpath) | |
} |
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Sorry about all the nits. I'm just trying to reduce the "if this, then this, otherwise that" aspect of this feature that was baked into the code. It adds unnecessary complexity for us and the user, and I'm hoping we can make this easier for everyone. |
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
No worries at all! I would love to make the helper message more clear |
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.
Thanks for your patience while we hammered out how this should work. This looks great and I think will be easier for people to use. 👍
* use user specified build directory if provided for porter manifest Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * update tests Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * update doc and fix tests Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * address comment Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * explicitly set default value for o.Dir Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * clearer help text Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
* Use user specified directory for resolving file path (#2142) * use user specified build directory if provided for porter manifest Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * update tests Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * update doc and fix tests Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * address comment Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * explicitly set default value for o.Dir Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * clearer help text Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Update to helm3 mixin v0.1.16 v0.1.16 includes fixes for using nonroot invocation images Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Sanitize archive folder name (#2154) * fix archive folder creation Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * replace path separator instead Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * modify test Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Adding pagination for installation, parameter, and credential list result using skip and limit option (#2137) * Add pagination option for installation list command using skip and limit flag Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Increase plugin start/stop timeouts As I was adding back in net/rpc plugins (the legacy v0 plugins), I realized that our plugin timeouts don't work well for net/rpc since it is much slower than gRPC. I've bumped both the plugin start and stop timeout defaults to make it less likely that a user will run into the timeout, while still giving us a good "oops the plugin is broken" timeout detection. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Add InstallationStore.FindInstallations (#2119) The advanced dependencies proposal needs to be able to search for installations based on more complex critieria than is available in the ListInstallations function (which is intended to support the porter installation list command). FindInstallations lets us craft any valid mongodb find query and execute it, returning a list of installations. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Rename DisplayRun.ClaimID to ID I missed this field when I did a sweep earlier to remove the use of the word claim in the release/v1 branch. In the rest of the CLI's output we call the run's id just ID or RunID, and should be consistent with that. I've changed DisplayID.ClaimID to ID so that we aren't exposing the term claim to our users (and it's not really the claim id anymore anyway). Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Support Docker TLS environment variables We are using the docker cli library to build images and I had thought this gave us automatic support for building against a remote docker host. It works fine for DOCKER_HOST, but turns out the TLS configuration environment variables are only parsed when the docker CLI flags are bound (which doesn't occur when we use it as a library). I've updated how we initialize the docker cli library so that DOCKER_TLS_VERIFY and DOCKER_CERT_PATH are picked up and passed to the library. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Add vet and lint targets to magefile Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Add ListOption input parameter struct and enable skip and limit option to credential and parameter list command as well Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Leave out default value for ListOption's properties Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Remove commented function signature Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Convert CreateListFilter to ToFindOptions method for ListOptions type receiver Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> Co-authored-by: Carolyn Van Slyck <me@carolynvanslyck.com> Co-authored-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Add state and status to list installation Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * fix archive folder test Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Fix Vet Errors (#2153) * Fix lint errors for unkeyed fields in composite literals Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com> * resolve lint errors on tags Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com> * Updated golden file to account for bad struct tag fix Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com> * Vet Fix: Rename example tests to use suffixes. Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com> * Replace ExtendedBundle{} initialization with a NewBundle constructor Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Improve error message loading wrong schema (#2157) * Improve error message loading wrong schema Signed-off-by: Kevin Barbour <kevinbarbourd@gmail.com> * Add myself to CONTRIBUTORS.MD Signed-off-by: Kevin Barbour <kevinbarbourd@gmail.com> * Don't use errors pkg, fix assert in test Signed-off-by: Kevin Barbour <kevinbarbourd@gmail.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Add prow github action This adds a prow github action that allows specified people (in the OWNERS file) to comment on a pull request with /lgtm to review the pull request, or /approve to merge the pull request. The github action handles executing the commands for you so that you don't need to have maintainer rights on the repository. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Switch prow to use pull_request instead of _target Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Updated installation schema with correct dependency schema Signed-off-by: Steven Gettys <s.gettys@f5.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * changed new manifest description for test Signed-off-by: Steven Gettys <s.gettys@f5.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Update k8s and containerd dependencies * Update to cnab-go v0.23.4 * Update containerd to v1.6.6 * Updated k8s to v0.24.1. This does not update docker since buildkit uses a funny unreleased version of docker. We won't be able to update to a new version of Docker until there's a release that has the new feature that buildkit relies upon. See go.mod for a link to the troublesome package in question. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Add comments Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * StateDefined as default value Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Move displayinstallation's state and status to metadata Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Add golden file test for print installation Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Add unit test for displayInstallation's state and status Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Change function name from set to get Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * Revert changes on test file Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * add new line Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * resolve conflict Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> * fix comment Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com> Co-authored-by: Yingrong Zhao <yingrong.zhao@gmail.com> Co-authored-by: Carolyn Van Slyck <me@carolynvanslyck.com> Co-authored-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com> Co-authored-by: Kevin Barbour <kevinbarbourd@gmail.com> Co-authored-by: Steven Gettys <s.gettys@f5.com>
What does this change
Currently, porter assumes current working directory is
pwd()
even if users have specified a directory through--dir
.Therefore using
--file
flag requires users to always provide the absolute file path to porter manifest file, or porter sill usepwd()
as the root directory when a relative path is provided.example:
The resolved porter manifest path would be
/home/test/hello/porter.yaml
This PR changes the behavior to use the directory specified by
--dir
as the root directory when resolving relative path provided by--file
.example:
The resolved porter manifest path would be
/home/test/example/hello/porter.yaml
What issue does it fix
Closes #1447
Notes for the reviewer
Checklist
Reviewer Checklist