feat: Add Workstation controller and mockgcp #3276
Conversation
|
|
||
| // +genclient | ||
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
| // TODO(user): make sure the pluralizaiton below is correct |
jasonvigil
force-pushed
the
workstations-workstation
branch
from
ea2ede5 to
cb02af3
Compare
|
/hold cancel |
jasonvigil
force-pushed
the
workstations-workstation
branch
from
cb02af3 to
d78b49e
Compare
jasonvigil
changed the title
There was a problem hiding this comment.
Suggest dropping the autogenerated code changes under ./pkg/clients/generated. We no longer update this lib since 1.125.
There was a problem hiding this comment.
Same as #3341 (comment), is it ok to update these (for consistency, for these older resources)? For any future resources, I will skip updating the pkg/clients/generated.
There was a problem hiding this comment.
Based on discussion in other PR, I reverted these changes also.
|
|
||
| // Find retrieves the GCP resource. | ||
| // Return true means the object is found. This triggers Adapter `Update` call. | ||
| // Return true means the object is not found. This triggers Adapter `Create` call. |
There was a problem hiding this comment.
Fixed, also updated the template.
| if mapCtx.Err() != nil { | ||
| return mapCtx.Err() | ||
| } | ||
| return updateOp.UpdateStatus(ctx, status, nil) |
There was a problem hiding this comment.
awesome. Thank you. Could you update the scifi template as well?
|
|
||
| req := &workstationspb.DeleteWorkstationRequest{Name: a.id.String()} | ||
| op, err := a.gcpClient.DeleteWorkstation(ctx, req) | ||
| if err != nil { |
There was a problem hiding this comment.
Does the GCP service returns any code if the resource does not exist (like 404)? If so, we may want to exclude NotFound from the error. If this case is already considered and covered in this PR, would you mind adding a comment?
There was a problem hiding this comment.
Nice catch. Fixed. Also noticed that WorkstationConfig and WorkstationCluster have this same issue, so I fixed those as well.
| spec: | ||
| parentRef: | ||
| name: workstationconfig-${uniqueId} | ||
| displayName: workstation-${uniqueId} |
There was a problem hiding this comment.
nit: make displayName a different value between create.yaml and update.yaml
There was a problem hiding this comment.
wait, is this an immutable field?
There was a problem hiding this comment.
Fixed. The displayName field is not immutable.
| spec: | ||
| parentRef: | ||
| name: workstationconfig-${uniqueId} | ||
| displayName: workstation-${uniqueId} |
There was a problem hiding this comment.
Fixed, displayName is not required. Removed from minimal test.
|
/approve A few nits. non blocker |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jasonvigil
force-pushed
the
workstations-workstation
branch
from
d78b49e to
b5041fd
Compare
Skip sending GCP API delete request if resource does not exist.
Return true if object is found, false if object is not found.
Also, it is not a required field. So, remove from minimal suite.
jasonvigil
force-pushed
the
workstations-workstation
branch
from
b5041fd to
b61af08
Compare
google-oss-prow
bot
merged commit 546d074
into
GoogleCloudPlatform:master
No description provided.