Modify apply logic in direct.go #128
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
size/S
atoato88
changed the title
|
I think current codes has two points to fix.
namespace is set at here, but this field is ignored, because SetObject is used here. namespace is used at here in For fixing this, we should not use
We should use a validator in applyOpts settings. |
|
I try to add some unit tests, but some features which are related to validator and namespace needs real k8s cluster. |
atoato88
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
Current PR set is just update function in direct.go, not includes tests related with this PR. |
|
/assign @justinsb |
atoato88
force-pushed
the
modify-direct-apply
branch
from
27863cc to
1b19c86
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: atoato88 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/L
atoato88
force-pushed
the
modify-direct-apply
branch
from
1b19c86 to
9f479ca
Compare
k8s-ci-robot
added
size/M
|
I've update code set, and this PR fix errors in guestbook-operator. |
| @@ -27,24 +26,36 @@ func (d *DirectApplier) Apply(ctx context.Context, | |||
| validate bool, | |||
| extraArgs ...string, | |||
| ) error { | |||
| tmpFile, err := ioutil.TempFile("", "tmp-manifest") | |||
There was a problem hiding this comment.
Do you think it'll be safer to have some randomness in the name?
I've a use case where two go routines use the applier on separate manifests at the same time.
With a constant name, there can be conflict.
A random name with a prefix and/or suffic may be safer?
I'm referring to the suffix example in https://golang.org/pkg/io/ioutil/#TempFile
| } | ||
| tmpFile.Write([]byte(manifest)) | ||
| tmpFile.Close() | ||
| tmpFile, err = os.Open(tmpFile.Name()) |
There was a problem hiding this comment.
We can have an error check for this.
BTW, does this not work if you assign tmpFile to os.Stdin directly, instead of closing and opening the same file? Both are *os.File for the same file.
|
@darkowlzz |
This commit changes a logic to apply in direct.go, because current codes ignores some features which are validate, etc.
atoato88
force-pushed
the
modify-direct-apply
branch
from
9f479ca to
a1fe1ee
Compare
|
Hi, any updates for this? We met similar issue because of the non-defaulting namespace. Taking the sample operator as an example (I manually added the namespace to all resources in manifest file so all resources should be deployed successfully), in the latest kubebuilder-declaritive-pattern, when the reconciled calls to update the Status of CR, it fails to find the CR while the CR is available when running Error in controller: Fetch CR in cluster Thanks |
|
@courageJ
As I said comment above, Current code of This error doesn't occur when use We needs to add Current code of this PR may fix this behavior, but we need a test code for this Apply function before this PR merge. |
|
@justinsb |
|
I tried to run the guestbook sample locally following the instruction here and ran into the following error: From error message I observed that |
| if err != nil { | ||
| return err | ||
| } | ||
| applyOpts.DeleteOptions = applyOpts.DeleteFlags.ToOptions(applyOpts.DynamicClient, applyOpts.IOStreams) |
There was a problem hiding this comment.
It looks like we overwrite DeleteOptions on line 69 below?
| @@ -53,6 +69,7 @@ func (d *DirectApplier) Apply(ctx context.Context, | |||
| applyOpts.DeleteOptions = &cmdDelete.DeleteOptions{ | |||
| IOStreams: ioStreams, | |||
| } | |||
| applyOpts.DeleteOptions.Filenames = []string{tmpFile.Name()} | |||
There was a problem hiding this comment.
Looking at the code, it seems that despite being in DeleteOptions.Filenames, this the filename of the objects we want to apply. Is that right? If that's right, do you mind adding a comment?
| if err != nil { | ||
| return err | ||
| } | ||
| tmpFile.Write([]byte(manifest)) |
There was a problem hiding this comment.
Nit: the error handling here is really tricky. I would split this into a function, like this:
func WriteTempFile(dir, pattern string, contents []byte) (string, error) {
tmpFile, err := ioutil.TempFile(dir, pattern)
if err != nil {
return "", err
}
success := false
defer func() {
if !success {
tmpFile.Close()
os.Remove(tmpFile.Name())
}
}()
if _, err := tmpFile.Write(contents); err != nil {
return "", err
}
if err := tmpFile.Close(); err != nil {
return "", err
}
success = true // don't remove the temp file
return tmpFile.Name(), nil
}
I'm happy to send a follow-on PR to add this if that's easier!
There was a problem hiding this comment.
Thank you to comment 😄
Current codes are something rough idea, so any review comments are very appreciate.
|
Sorry for the (huge) delay here. I think I finally understood what was going on - I was very confused by DeleteOptions.Filenames. If I'm right as to what it is for, a comment there would be very helpful! |
|
Thank you to comment. |
|
I was able to reproduce the namespace problem with the direct-applier and I uploaded a fix for that - #173. I'm guessing you achieve that by having the Apply logic parse the yaml, which includes the call to |
|
@atoato88: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-rebase
|
/close As of now #173 is merged which updates the apply logic. |
|
@atoato88: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This PR changes a logic to apply in
direct.go, because current codes ignores some features which are validate and namespace defaulting, etc.I would like to add tests for this topics later.