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

Improve auto-detect of name vs filepath for Cred/Param Sets #1217

Closed
vdice opened this issue Aug 18, 2020 · 2 comments
Closed

Improve auto-detect of name vs filepath for Cred/Param Sets #1217

vdice opened this issue Aug 18, 2020 · 2 comments
Labels
2 - 🍕 Pizza should be eaten daily bug Oops, sorry! gap We missed a spot help wanted Good for someone who has contributed before
Milestone

Comments

@vdice
Copy link
Member

vdice commented Aug 18, 2020

We can/should improve the auto-detection logic used for consuming Credential and Parameter Sets on the command line, e.g. porter install -p paramset or porter install -p paramset.json. Primarily, it would be great to solve the case of whether or not the argument is a named cred set or a file existing in the current directory (and sans any filepath separators).

This arose via feedback from #1213:

@vdice, The examples you provided were enough to help me figure out that my issue had indeed been with the parameter auto-detection syntax.

If I have a parameter file in my working directory, testparams.json, then apparently
porter upgrade -p testparams.json
will interpret testparams.json as a name. Whereas if I run
porter upgrade -p ./testparams.json
it will interpret that as a path,

and if I have testparams.json in a subdirectory (params)
porter upgrade -p params/testparams.json
will interpret as a file path.

I think this autodetection behavior is a little surprising, but I'm satisfied that the functionality I was looking for still exists. Thank you for the explanation.

Originally posted by @CSberger in #1213 (comment)

@vdice vdice added the gap We missed a spot label Aug 18, 2020
@vdice vdice added the help wanted Good for someone who has contributed before label Aug 18, 2020
@carolynvs
Copy link
Member

carolynvs commented Oct 15, 2020

Right now the code looks for a filepath separator. A couple other things we can do:

  1. Look for ".json" too
  2. Try it as a file first, and if so, just treat it as a file and otherwise it's a name of a credential set
  3. Split the flag to be explicit

I'm not saying we should do all of these, just identifying some options. Personally I think #2 may be able to replace what we do now with some handling for if they gave us a relative or absolute path.

@carolynvs carolynvs added 2 - 🍕 Pizza should be eaten daily bug Oops, sorry! labels Jan 5, 2021
@carolynvs carolynvs added this to the 1.0 milestone Jan 28, 2021
@carolynvs
Copy link
Member

Let's implement the second suggestion above, where when we load a parameter set, first we check if it is a file on the local filesystem. If so load the parameter set from the file. Otherwise assume it is a parameter set name and load it normally.

The impacted code is located at

// If name looks pathy, attempt to load from a file
// Else, read from Porter's parameters store
if strings.Contains(name, string(filepath.Separator)) {
pset, err = p.loadParameterFromFile(name)
} else {
pset, err = p.Parameters.Read(name)
}

fibonacci1729 pushed a commit to fibonacci1729/porter that referenced this issue Aug 23, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Signed-off-by: Brian Hardock <[email protected]>
carolynvs added a commit that referenced this issue Aug 25, 2021
Signed-off-by: Brian Hardock <[email protected]>

Co-authored-by: Brian Hardock <[email protected]>
Co-authored-by: Carolyn Van Slyck <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - 🍕 Pizza should be eaten daily bug Oops, sorry! gap We missed a spot help wanted Good for someone who has contributed before
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants