-
Notifications
You must be signed in to change notification settings - Fork 64
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
Rename workspace to devworkspace #396
Conversation
78d4916
to
546111a
Compare
546111a
to
9aff1eb
Compare
// Id of the workspace | ||
WorkspaceId string `json:"workspaceId"` | ||
// Id of the devworkspace | ||
DevWorkspaceId string `json:"devworkspaceId"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: there is a place for inconsistency: DevWorkspaceId but devworkspaceId.
Consistent ways:
- DevWorkspaceId -> devWorkspaceId
- DevworkspaceId -> devworkspaceID
I don't like how DevworkspaceId
looks like.
Angel and Josh vote for devworkspaceId
instead of devWorkspaceId
.
Let me know if you think we should go with any of consistent ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go might complain that the json string doesn't match the variable name, but that's just Go being pedantic.
// Id of the workspace | ||
WorkspaceId string `json:"workspaceId"` | ||
// Id of the devworkspace | ||
DevWorkspaceId string `json:"devworkspaceId"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go might complain that the json string doesn't match the variable name, but that's just Go being pedantic.
@@ -4,7 +4,7 @@ import ( | |||
corev1 "k8s.io/api/core/v1" | |||
) | |||
|
|||
type WorkspacePodContributions struct { | |||
type DevWorkspacePodContributions struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this rename isn't strictly necessary, but if it's a hassle to undo then I'm fine with it. Is this struct used by anyone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only DWO who uses it.
I can return it back but personally I would like a bit more to use DevWorkspace to be synced with CR kind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW File name should be synced with type )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I checked and it's unused. DWO has it's own impl https://github.com/devfile/devworkspace-operator/blob/main/apis/controller/v1alpha1/common.go#L18
If it's unused, let's remove it then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a strange thing to have in the spec API so I'm 👍 to remove as long as odo isn't using it.
@@ -1,4 +1,4 @@ | |||
apiVersion: workspace.che.eclipse.org/v1alpha2 | |||
apiVersion: workspace.devfile.io/v1alpha2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
Good catch
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amisevsk, sleshchenko 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 |
New changes are detected. LGTM label has been removed. |
8eec3a4
to
35cde39
Compare
@amisevsk I checked WTO updating after renaming and it does not work. Existing CRs must be valid according to the schema. It would work if devWorkspaceId is optional, but it's not case now. Reverted changes in v1alpha1. |
35cde39
to
f533543
Compare
What does this PR do?
This PR finalize workspace to DevWorkspace renaming. So, I mainly did next replacement:
The only model change that is done DevWorkspace.status.workspaceId is renamed to
devworkspaceId
.Everything else is just docs changes with workspace -> devworkspace replacement, and a few go types renaming.
We still use workspace for two pieces:
workspace.devfile.io
K8s API group, since it's not possible to rename in backward compatible manner, but it's not a big deal.devfile-api/pkg/apis/workspaces
go package. If we rename it - it's again breaking change and we should adapt depending projects, like http://github.com/devfile/library.Note: v1alpha1 is not touched since it will fail updated with OLM, and it's critical for WTO.
So, there are still workspace words(131).
If you want to check them, the following regexp should help you:
grep -r "[^v-][wW]orkspace" .
output of: grep -r "grep -r "[^v-][wW]orkspace" ."
What issues does this PR fix or reference?
devfile/devworkspace-operator#321
Is your PR tested? Consider putting some instruction how to test your changes
Testing will be done on DWO side.
Docs PR