bump toolchain-api, minor fixes#234
bump toolchain-api, minor fixes#234openshift-merge-bot[bot] merged 5 commits intokonflux-workspaces:mainfrom
Conversation
Signed-off-by: Francesco Ilario <filario@redhat.com>
|
Skipping CI for Draft Pull Request. |
Signed-off-by: Francesco Ilario <filario@redhat.com>
Signed-off-by: Francesco Ilario <filario@redhat.com>
dperaza4dustbit
left a comment
dperaza4dustbit
left a comment
There was a problem hiding this comment.
Looks good one question though
| _, err := controllerutil.CreateOrUpdate(ctx, r.Client, w, func() error { | ||
| w.Spec.DisplayName = "default" | ||
| w.Spec.Visibility = workspacesv1alpha1.InternalWorkspaceVisibilityPrivate | ||
| if w.ObjectMeta.CreationTimestamp.IsZero() { |
There was a problem hiding this comment.
What is this check doing, making sure this is not an update event?
There was a problem hiding this comment.
exactly, this way we set this fields only when creating the resource. Otherwise we will reset visibility and display name of the workspace every time an UserSignup is updated/reconciled
There was a problem hiding this comment.
We could hoist these assignments out of this closure. They'll be overwritten if the resource exists and has those fields set.
There was a problem hiding this comment.
that may work, I got inspired from the controller-runtime example: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/controller/controllerutil#CreateOrUpdate. Not a strong preference, but I'd prefer the one used in doc.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: filariow, sadlerap The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
No description provided.