feat: allow kb layout works on operator-sdk run local command#2948
feat: allow kb layout works on operator-sdk run local command#2948camilamacedo86 merged 7 commits intooperator-framework:masterfrom camilamacedo86:cmd-run
Conversation
|
|
||
| log "github.com/sirupsen/logrus" | ||
| "github.com/spf13/pflag" | ||
| logf "sigs.k8s.io/controller-runtime/pkg/log" |
There was a problem hiding this comment.
just sort the imports and added the kbutils one.
| func (c runLocalArgs) run() error { | ||
| log.Infof("Running the operator locally in namespace %s.", c.watchNamespace) | ||
|
|
||
| projutil.MustInProjectRoot() |
There was a problem hiding this comment.
it was been called inside of each run. It is a required check for all types.
There was a problem hiding this comment.
This can actually be removed from here I think. We call this first thing in the operator-sdk run command here:
operator-sdk/cmd/operator-sdk/run/cmd.go
Line 73 in 48b7203
There was a problem hiding this comment.
Verified. Yes, you are right, we can remove it.
| var dc *exec.Cmd | ||
| // Get the args that will be used to exec the binary. | ||
| // Users are allowed to use the flag operator-flags to pass any value that they may wish | ||
| args := c.buildArgsBasedOnOperatorFlag() |
There was a problem hiding this comment.
Since I need to check how it works, I extracted the logic in funcs and then, added comments to make easier we understand and keep it maintained.
camilamacedo86
added
the
kubebuilder-integration
hasbro17
left a comment
hasbro17
left a comment
There was a problem hiding this comment.
Some nits on the refactoring not related to the alignment but the actual alignment seems okay.
LGTM
| fs.StringVar(&c.watchNamespace, "watch-namespace", "", | ||
| prefix+"The namespace where the operator watches for changes. Set \"\" for AllNamespaces, "+ | ||
| "set \"ns1,ns2\" for MultiNamespace") |
There was a problem hiding this comment.
For the first contidional here It will set watch-namepace == namespace if the flag is set. Since the flag does not exist when it is in the new layout all is fine. Anyway, we can be redundant and add a check as well to ensure case something change.
However, for the second conditional here you are right we need set it for cluster-scoped.
| cmd = exec.Command(binName, args...) | ||
| } | ||
|
|
||
| // Kills the binary execution, if the exit signal be raised from the container |
There was a problem hiding this comment.
operator-sdk run --local is not running in a container.
| // Kills the binary execution, if the exit signal be raised from the container | |
| // Kill the command if an exit signal is received. |
| // Add default env vars and values informed via flags | ||
| c.addEnvVars(cmd) | ||
| if err := projutil.ExecCmd(cmd); err != nil { | ||
| return err |
There was a problem hiding this comment.
Keep the error context
| return err | |
| return fmt.Errorf("failed to run operator locally: %v", err) |
There was a problem hiding this comment.
I removed it before because it will be : failed to run operator locally: failed to run operator locally: twice see here. However, I did the change as you suggested.
There was a problem hiding this comment.
Ah! Didn't realize this was already duplicated. In that case, feel free to remove, either here or in a follow-up.
| var outputBinName, mainPath string | ||
| if kbutil.HasProjectFile() { | ||
| outputBinName = filepath.Join(kbutil.BinBuildDir, projutil.GetProjectName()+"-local") | ||
| mainPath = kbutil.MainPath |
There was a problem hiding this comment.
We compile the entire main package, not just main.go since it is possible for multiple go files to exist in one directory and be in the main package.
We should use projutil.GetGoPkg() as the path (following the pattern for what we're doing for SDK-scaffolded projects), and for clarity, I suggest renaming the variable to packagePath to align with the GoCmdOptions struct
| mainPath = kbutil.MainPath | |
| packagePath = projutil.GetGoPkg() |
| Args: c.getBuildRunLocalArgs(), | ||
| } | ||
| if err := projutil.GoBuild(opts); err != nil { | ||
| return outputBinName, err |
There was a problem hiding this comment.
If we failed to build the binary, we shouldn't return the outputBinName
| return outputBinName, err | |
| return "", err |
joelanford
left a comment
joelanford
left a comment
There was a problem hiding this comment.
A few more nits/fixes. LGTM once they're addressed.
3 participants
Description of the change:
Note: All flags can work successfully with kb layout as well. However, we decide at the first moment hidden the
--namespace(deprecated) and the--watch-namespaceflags.Motivation for the change:
KB integration
Local Tests
Following a summary.