feat(ws): Notebooks 2.0 // Backend // Envtest creation #403
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
yehudit1987
force-pushed
the
feat/#380
branch
8 times, most recently
from
61f6227 to
1f03d2b
Compare
… envtest Signed-off-by: Yehudit Kerido <[email protected]>
| kubeconfig.QPS = float32(cfg.ClientQPS) | ||
| kubeconfig.Burst = cfg.ClientBurst |
There was a problem hiding this comment.
What is the significance of setting this values here (irrespective of --enable-envtest flag) - when inside the Factory we only define these values for "real" k8s:
Feels like these lines shouldn't be here (and any needs to set them should be handled within GetManagerAndConfig function) 🤔
| } | ||
| f.logger.Info("Local dev K8s API (envtest) is ready.", "host", cfg.Host) | ||
|
|
||
| if testEnvInstance != nil { |
There was a problem hiding this comment.
What could cause testEnvInstance to be nil at this point in the code?
If this is a possible outcome...
- seems wrong to print the "ready" message on L81
- seems like there should be an
elseblock helping user understand there could be an issue (or should we exit application at that point?)
| func (f *ClientFactory) GetClient(ctx context.Context) (client.Client, func(), error) { | ||
| mgr, _, cleanup, err := f.GetManagerAndConfig(ctx) | ||
| if err != nil { | ||
| return nil, cleanup, err |
There was a problem hiding this comment.
I wonder if err is defined here.. if we should attempt to invoke the cleanup function at this point.. instead of returning it to the calling code (presumably where it must be invoked)... as then forgetting to invoke cleanup in calling code could cause a problem?
| ctrl.Log.Info("Local dev environment stopped.") | ||
| } | ||
|
|
||
| func getProjectRoot() (string, error) { |
There was a problem hiding this comment.
I am still myself upskilling my knowledge in Golang - so take what I say with a grain of salt!
I worry the reliance on os.Getwd() could lead to strange failure patterns...
From (quick) research - I feel something like the following may be more robust - but open to discussion!
import (
"go/build"
"golang.org/x/mod/modfile"
)
func getProjectRoot() (string, error) {
// First try to find the module root
if modRoot := os.Getenv("GOMOD"); modRoot != "" {
return filepath.Dir(modRoot), nil
}
// Fall back to build.Import
pkg, err := build.Import("github.com/kubeflow/notebooks", "", build.FindOnly)
if err != nil {
return "", err
}
return pkg.Dir, nil
}
example:
➜ backend/ git:(feat/#380) $ go env GOMOD
/Users/astonebe/Development/Code/GitHub/kubeflow-notebooks/workspaces/backend/go.mod
| return nil | ||
| } | ||
|
|
||
| func loadAndCreateWorkspaceKindsFromDir(ctx context.Context, cl client.Client, |
There was a problem hiding this comment.
FOR DISCUSSION - NOT ADVOCATING FOR CHANGES QUITE YET
I wonder if instead of relying on YAML files that exist in source control.. coupled then with all this application logic.. if it would just be easier to adopt an approach similar to what the controller is doing in its tests
If we defined everything programmatically... I imagine the code to construct the workspaces would be easier to understand/implement as we can rely on const values (or at minimum variables defined purely within the application logic) and not need to parse out values from querying the control plan..
Thoughts?
| } | ||
|
|
||
| // createInitialResources creates namespaces, WorkspaceKinds, and Workspaces. | ||
| func createInitialResources(ctx context.Context, cl client.Client) error { |
There was a problem hiding this comment.
Given the initial limited scope of this work - I am comfortable with the answer being "we can address this in a follow up item" ...
But with that disclaimer in mind - I was expecting to see something around volumes here - since the Workspaces being created have reference to a volume.
Thoughts?
This PR solve issue #380