kfctl support relative path #4331
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jlewi
reviewed
Oct 17, 2019
bootstrap/pkg/utils/k8utils.go
Outdated
|
|
||
| log.Infof("Downloading %v to %v", configFile, appFile) | ||
| err = gogetter.GetFile(appFile, configFile) | ||
| // Check if configFile is a remote file. |
There was a problem hiding this comment.
Can we refactor the code to make this easier to test?
One thought would be to define a function isRemoteFile and then we could add a unittest to cover the cases
- remote URI
- relative path
- absolute path.
|
test error: related to kubeflow/testing#490? |
|
/retest |
1 similar comment
|
/retest |
jlewi
reviewed
Oct 18, 2019
| } | ||
|
|
||
| // If the config file is a remote URI, check to see if the current directory | ||
| // is empty because we will be generating the KfApp there. | ||
| cwd := "" |
There was a problem hiding this comment.
why initialie to "" as opposed to getCwd()?
There was a problem hiding this comment.
Is this the current directory or is it a bool indicating whether its empty?
There was a problem hiding this comment.
nit: Should we clean this code up a bit? isCwdEmpty is returning the current directory but only if its empty.
That's a bit convulated. Should we change isCwdEmpty to isDirEmpty(cwd) and return a bool
jlewi
reviewed
Oct 18, 2019
bootstrap/pkg/utils/k8utils.go
Outdated
| Code: int(kfapis.INVALID_ARGUMENT), | ||
| Message: fmt.Sprintf("Error parsing file path: %v", err), | ||
| } | ||
| } else { |
There was a problem hiding this comment.
nit: move the code out of the else block since code in the if statement will never reach it.
jlewi
reviewed
Oct 18, 2019
| filePath: "http://github.com", | ||
| isRemote: true, | ||
| }, | ||
| { |
There was a problem hiding this comment.
nit: add the test case "abc.txt"
jlewi
suggested changes
Oct 18, 2019
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @jlewi, @lluunn, and @richardsliu)
bootstrap/pkg/kfapp/coordinator/coordinator.go, line 352 at r2 (raw file):
if isRemoteFile { cwd, err = os.Getwd() if err != nil {
I think we can replace this if/else block and just do
c.kfDef.Spec.Appdir= path.Dir(appFile)
At this point appFile should be the path to a local File because
- Either cfgFile is a local file and we set appFile=localFile
- We copied the remote file to the current directory and appFile is the local path it was copied to.
|
/hold |
|
/hold cancel |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlewi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
saffaalvi
pushed a commit
to StatCan/kubeflow
that referenced
this pull request
Feb 11, 2021
* support relative path * refactor util, add unit test * address review * fix * fix
saffaalvi
pushed a commit
to StatCan/kubeflow
that referenced
this pull request
Feb 12, 2021
* support relative path * refactor util, add unit test * address review * fix * fix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
use neturl.Parse instead of ParseRequestURI because the latter only works with abs path.
https://golang.org/pkg/net/url/#ParseRequestURI
/cc @jlewi @richardsliu
kubeflow/kfctl#49
This change is