feat: Add dry-run functionality for WorkspaceKind creation

[bhaktinarvekar]

No description provided.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign thesuperzapper for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[andyatmiami]

This is a good start - so I appreciate your contributions! But we have a little work to do in order to unlock the desired functionality.

Feel free to reach out to me in Slack if you need/want to discuss further.

[andyatmiami]

Can you explain your rationale for making this change to NOT call ValidateContentType ? I'm assuming its a desire to outright fail in the presence of additionally parameters declared in Content-Type header ?

i.e. application/yaml; charset=utf-8

• passes in ValidateContentType
• fails in your custom implementation

If that is the primary motivation - I would opt to not deviate from the common helper function we have created and used consistently throughout the codebase. So, I would recommend undoing this change and leaving this particular piece of logic as-is.

If I am misunderstanding the motivation behind the change - or you feel its critical to implement something like this - lets talk about it - but ideally with the goal of supporting this in the common helper function and not fragmenting the codebase w.r.t. content type validation.

[andyatmiami]

I appreciate this is the first time we are supporting a query parameter across any of our APIs - but it won't be the last! As such - I think its warranted to consider adding a helper function to workspaces/backend/api/helpers.go.
admittedly, I'm not immediately sure how to structure this for proper abstraction - presently leaning towards:

func (a *App) GetBooleanQueryParameter(w http.ResponseWriter, r *http.Request, paramName string) (bool, error) {
	value := r.URL.Query().Get(paramName)
	if value == "" {
		return false, nil // parameter not present, default to false
	}

	if value != "true" && value != "false" {
		err := fmt.Errorf("Invalid query parameter '%s' value '%s'. Must be 'true' or 'false'", paramName, value)
		a.badRequestResponse(w, r, err)
		return false, err
	}

	return value == "true", nil
}

calling code would then look like:

isDryRun, err := a.GetBooleanQueryParameter(w, r, "dry_run")
if err != nil {
    return
}

[andyatmiami]

ℹ️ Note: strconv.ParseBool(...) not being leveraged here (which I agree with) - as it will also properly parse 0, 1, etc as boolean values - which feels wrong to support in an API.

[andyatmiami]

The a.dataResponse and a.createdResponse helper functions already handle setting this Content-Type header..

• https://github.com/kubeflow/notebooks/blob/notebooks-v2/workspaces/backend/api/helpers.go#L50

Was there something you observed that made it necessary to explicitly set this here? I would imagine we should remove this logic as its redundant.. but please correct me if I am wrong!

[andyatmiami]

As implemented - this is NOT achieving the desired outcome as written in the issue... primarly:

a dry_run is performed at the kubernetes layer (see: CreateOptions , UpdateOptions )

This code simply "echoes back" what was provided on the request - which won't inform the user if there were issues in the provided payload.

Even when dryRun is true - we STILL want to call r.client.Create - but we want to invoke it in such a way that the controller-runtime will evaluate it with dry_run semantics..

Something like the following is what I would expect to see in the implementation

    var createOptions []client.CreateOption
    
    if dryRun {
        // Add dry-run option to the Kubernetes client call
        createOptions = append(createOptions, client.DryRunAll)
    }
    
    // Always call the Kubernetes client, with or without dry-run options
    if err := r.client.Create(ctx, workspaceKind, createOptions...); err != nil {

and then in testing this against a live cluster.. you should be able to see errors returned from k8s itself in the event you provided an invalid YAML

[andyatmiami]

⚠️ Please note this will then significantly impact the tests provided in this PR to support the desired behavior.

[andyatmiami]

We should preserve this TODO comment as it will still hold true even with the merge of this PR

i.e. once available - we would want a WorkspaceKindUpdate being returned from this function

[andyatmiami]

This file should not be included in this PR - as none of the actual changes implemented affect the workspaces/frontend directory.

I am guessing this is a side effect of the extreme amount of commits that are erroneously getting included in the PR. Feel free to reach out to me in Slack if you need help getting your source branch cleaned up!