Skip to content

Finalize workspace -> devWorkspace renaming #322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 8, 2021

Conversation

sleshchenko
Copy link
Member

@sleshchenko sleshchenko commented Mar 15, 2021

What does this PR do?

This PR is supposed to finalize workspace -> devWorkspace renaming but it does not do it fully.

What issues does this PR fix or reference?

It's for #321

Che Machine exec PR eclipse-che/che-machine-exec#138

Is it tested? How?

not yet.

@openshift-ci-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

// WorkspaceIDLabel is label key to store workspace identifier
WorkspaceIDLabel = "controller.devfile.io/workspace_id"
// DevWorkspaceIDLabel is label key to store workspace identifier
DevWorkspaceIDLabel = "controller.devfile.io/devworkspace_id"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think id would be enough

Copy link
Member Author

@sleshchenko sleshchenko Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the time to make this label shorter if we wish, but I don't think we win anything with such change, just make things less clearer.
It would be clearer if it would be devworkspace.devfile.io/name but it's controller.devfile.io/ group, and just name is not clear IMHO

Also this label is propagated to all devworkspace related stuff, pod, service, route.... It may be not clear enough to see controller.devfile.io/name label.

WorkspaceId string `json:"workspaceId"`
// Class of the routing: this drives which Workspace Routing controller will manage this routing
// Id for the DevWorkspace being routed
DevWorkspaceId string `json:"devWorkspaceId"`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

devworkspaceId vs devWorkspaceId

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on DevWorkspaceId as go field, but json version is devworkspaceId

@sleshchenko
Copy link
Member Author

/test v5-devworkspaces-operator-e2e

1 similar comment
@sleshchenko
Copy link
Member Author

/test v5-devworkspaces-operator-e2e

Copy link
Collaborator

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested updating from main to this PR (manually) and saw no issues except from some strangeness that I think was my cluster rather than the operator.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, sleshchenko

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:
  • OWNERS [amisevsk,sleshchenko]

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

@sleshchenko sleshchenko force-pushed the workspace2devWorkspace branch from d690dc5 to 47dbfb6 Compare April 8, 2021 11:40
@openshift-ci-robot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@sleshchenko
Copy link
Member Author

Tested failed because I forget to rename workspace_name to devworkspace_name label.
Now it should be fine. Ran e2e against RHPDS cluster.

@sleshchenko
Copy link
Member Author

Found a bug. Deployment must be recreated if selector is changed.
Otherwise workspace won't start due:

2021-04-08T11:45:29.354Z        ERROR   controller      Reconciler error        {"reconcilerGroup": "workspace.devfile.io", "reconcilerKind": "DevWorkspace", "controller": "devworkspace", "name": "spring-petclinic", "namespace": "opentlc-mgr-che", "error": "Deployment.apps \"workspace8313386c181c4e11\" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{\"controller.devfile.io/devworkspace_id\":\"workspace8313386c181c4e11\", \"controller.devfile.io/devworkspace_name\":\"spring-petclinic\"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable"}

@sleshchenko sleshchenko force-pushed the workspace2devWorkspace branch from 47dbfb6 to f84785f Compare April 8, 2021 12:09
@sleshchenko sleshchenko force-pushed the workspace2devWorkspace branch from f84785f to a4659ef Compare April 8, 2021 12:11
@sleshchenko
Copy link
Member Author

I tested all changed components together: DWO, DWCO, Che Theia, Che Machine Exec, Che Dashboard and it seems to work

@sleshchenko
Copy link
Member Author

/test v5-devworkspaces-operator-e2e

@sleshchenko
Copy link
Member Author

I tested and it works, merging but @amisevsk @JPinkney you have any comments to the last commits - let me know and I'll fix it.

@sleshchenko sleshchenko merged commit c6a22a3 into main Apr 8, 2021
@sleshchenko sleshchenko deleted the workspace2devWorkspace branch April 8, 2021 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants