kfctl mirror should recourse over dependencies #342
Conversation
|
This change is |
|
/assign @yanniszark |
|
Actually it looks like we can parameterize the functions: So we shouldn't actually need to add annotations to all the objects we can just add an appropriate parameter to the function. |
|
/hold I'm going to split this up into two PRs
|
jlewi
changed the title
jlewi
changed the title
|
@yanniszark this is ready for review. I scoped down the PR as mentioned in my earlier comment. |
|
/hold cancel |
|
/test all |
|
@jlewi thanks a lot for taking the time to improve |
|
/assign @Tomcli @Tomcli Could you review this please? /unassign @yanniszark |
|
/assign @adrian555 Could you PTAL? |
|
/assign @pdmack |
There was a problem hiding this comment.
Reviewed 9 of 11 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jlewi and @Tomcli)
pkg/mirror/mirror_image.go, line 210 at r2 (raw file):
} p := path.Join(absPath, r)
maybe check whether r is a filepath first?
pkg/mirror/mirror_image.go, line 215 at r2 (raw file):
log.Errorf("Error occurred while processing %v; error %v", p, err) } }
According to kustomize definition of resources, it can include file, path and uri. This code does not handle uri case. And the file can be json too. Fine if they are not possible/expected in Kubeflow scenario. But maybe better add a comment here.
pkg/utils/diff.go, line 65 at r2 (raw file):
} return "X" }
Have seen this code a couple of places. Wondering if somehow they can be put in a common package to be shared.
|
@jlewi sorry for the delay, reviewed above. Thanks. |
There was a problem hiding this comment.
Reviewable status: 10 of 12 files reviewed, 3 unresolved discussions (waiting on @adrian555 and @Tomcli)
pkg/mirror/mirror_image.go, line 210 at r2 (raw file):
Previously, adrian555 (Adrian Zhuang) wrote…
maybe check whether
ris a filepath first?
Good catch
pkg/mirror/mirror_image.go, line 215 at r2 (raw file):
Previously, adrian555 (Adrian Zhuang) wrote…
According to kustomize definition of resources, it can include file, path and uri. This code does not handle uri case. And the file can be json too. Fine if they are not possible/expected in Kubeflow scenario. But maybe better add a comment here.
Checked if its remote
pkg/utils/diff.go, line 65 at r2 (raw file):
Previously, adrian555 (Adrian Zhuang) wrote…
Have seen this code a couple of places. Wondering if somehow they can be put in a common package to be shared.
I think that's a good idea long term and added a TODO. Its a bit of work to refactor it into its own go module. I don't think we would want to reuse it without putting it in its own go module. Right now we would pull in all the dependencies for kfctl which slows down builds of very simple utilities. So in many cases it can be easier to just copy paste.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jlewi and @Tomcli)
pkg/mirror/mirror_image.go, line 213 at r3 (raw file):
p := path.Join(absPath, r) if b, err :=utils.IsRemoteFile(p); b || err != nil {
add space after :=
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jlewi and @Tomcli)
pkg/mirror/mirror_image.go, line 230 at r3 (raw file):
p := path.Join(absPath, r) if b, err :=utils.IsRemoteFile(p); b || err != nil {
add space after :=
|
Thanks @adrian555 I ran go fmt. |
* The current implementation of kfctl mirror was not looking
at dependent packages that were out of tree. This change
causes the mirror command to look at the resources directive
inside the kustomization.yaml and follow those links.
* This is necessary to support blueprints and some of the refactoring
of the kustomize manifests intended to better support Day 2 operations.
* Fix kubeflow#340 kfctl mirror shouldn't hard code the directory to search
for images but instead allow it to be provided via a flag.
* mirror pipelines should only apply the exclude prefix if it is the
non empty string.
* exclude will be empty if we want to match all images.
* On error print stack trace to help locate source of error.
* If filename is empty throw an error as well.
* kfctl mirror needs to check bases as well.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrian555 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 |
* The current implementation of kfctl mirror was not looking
at dependent packages that were out of tree. This change
causes the mirror command to look at the resources directive
inside the kustomization.yaml and follow those links.
* This is necessary to support blueprints and some of the refactoring
of the kustomize manifests intended to better support Day 2 operations.
* Fix kubeflow#340 kfctl mirror shouldn't hard code the directory to search
for images but instead allow it to be provided via a flag.
* mirror pipelines should only apply the exclude prefix if it is the
non empty string.
* exclude will be empty if we want to match all images.
* On error print stack trace to help locate source of error.
* If filename is empty throw an error as well.
* kfctl mirror needs to check bases as well.
* The current implementation of kfctl mirror was not looking
at dependent packages that were out of tree. This change
causes the mirror command to look at the resources directive
inside the kustomization.yaml and follow those links.
* This is necessary to support blueprints and some of the refactoring
of the kustomize manifests intended to better support Day 2 operations.
* Fix kubeflow#340 kfctl mirror shouldn't hard code the directory to search
for images but instead allow it to be provided via a flag.
* mirror pipelines should only apply the exclude prefix if it is the
non empty string.
* exclude will be empty if we want to match all images.
* On error print stack trace to help locate source of error.
* If filename is empty throw an error as well.
* kfctl mirror needs to check bases as well.
* The current implementation of kfctl mirror was not looking
at dependent packages that were out of tree. This change
causes the mirror command to look at the resources directive
inside the kustomization.yaml and follow those links.
* This is necessary to support blueprints and some of the refactoring
of the kustomize manifests intended to better support Day 2 operations.
* Fix kubeflow#340 kfctl mirror shouldn't hard code the directory to search
for images but instead allow it to be provided via a flag.
* mirror pipelines should only apply the exclude prefix if it is the
non empty string.
* exclude will be empty if we want to match all images.
* On error print stack trace to help locate source of error.
* If filename is empty throw an error as well.
* kfctl mirror needs to check bases as well.
* The current implementation of kfctl mirror was not looking
at dependent packages that were out of tree. This change
causes the mirror command to look at the resources directive
inside the kustomization.yaml and follow those links.
* This is necessary to support blueprints and some of the refactoring
of the kustomize manifests intended to better support Day 2 operations.
* Fix kubeflow#340 kfctl mirror shouldn't hard code the directory to search
for images but instead allow it to be provided via a flag.
* mirror pipelines should only apply the exclude prefix if it is the
non empty string.
* exclude will be empty if we want to match all images.
* On error print stack trace to help locate source of error.
* If filename is empty throw an error as well.
* kfctl mirror needs to check bases as well.
* The current implementation of kfctl mirror was not looking
at dependent packages that were out of tree. This change
causes the mirror command to look at the resources directive
inside the kustomization.yaml and follow those links.
* This is necessary to support blueprints and some of the refactoring
of the kustomize manifests intended to better support Day 2 operations.
* Fix kubeflow#340 kfctl mirror shouldn't hard code the directory to search
for images but instead allow it to be provided via a flag.
* mirror pipelines should only apply the exclude prefix if it is the
non empty string.
* exclude will be empty if we want to match all images.
* On error print stack trace to help locate source of error.
* If filename is empty throw an error as well.
* kfctl mirror needs to check bases as well.
kfctl mirror should recourse over dependencies
The current implementation of kfctl mirror was not looking
at dependent packages that were out of tree. This change
causes the mirror command to look at the resources directive
inside the kustomization.yaml and follow those links.
of the kustomize manifests intended to better support Day 2 operations.
Fix kfctl mirror images should not hardcode the kustomize directory #340 kfctl mirror shouldn't hard code the directory to search
for images but instead allow it to be provided via a flag.