Skip to content
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

feat: support workspace defaults #1395

Merged
merged 6 commits into from
Jun 22, 2022
Merged

feat: support workspace defaults #1395

merged 6 commits into from
Jun 22, 2022

Conversation

olexii4
Copy link
Contributor

@olexii4 olexii4 commented Jun 2, 2022

Signed-off-by: Oleksii Orel oorel@redhat.com

What does this PR do?

Add support for workspace default editor and workspace default components.

Example:

spec:
  server:
    workspaceDefaultComponents:
      - name: universal-developer-image
        container:
            image: quay.io/devfile/universal-developer-image:ubi8-latest
    workspaceDefaultEditor: eclipse/che-theia/next

Screenshot/screencast of this PR

What issues does this PR fix or reference?

It needs for eclipse-che/che#21353

How to test this PR?

cat EOF << > /tmp/patch.yaml
<PATCH_CONTENT>
EOF

chectl server:deploy \
     --installer operator \
     --platform <PLATFORM_TO_DEPLOY> \
     --che-operator-image <CUSTOM_OPERATOR_IMAGE> \
     --che-operator-cr-patch-yaml /tmp/patch.yaml

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

api/v1/checluster_types.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #1395 (3bfa497) into main (08de5aa) will not change coverage.
The diff coverage is n/a.

❗ Current head 3bfa497 differs from pull request most recent head b657f2d. Consider uploading reports for the commit b657f2d to get more accurate results

@@           Coverage Diff           @@
##             main    #1395   +/-   ##
=======================================
  Coverage   63.27%   63.27%           
=======================================
  Files          70       70           
  Lines        5827     5827           
=======================================
  Hits         3687     3687           
  Misses       1777     1777           
  Partials      363      363           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08de5aa...b657f2d. Read the comment docs.

@ibuziuk ibuziuk requested a review from amisevsk June 3, 2022 09:10
Copy link
Member

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

@amisevsk @l0rd could you please review in conjunction with devfile/devworkspace-operator#844
are we good with the format described in the eclipse-che/che#21353

As I recall last time we discussed more explicit definition - defaultContainer and defaultEditor

@ibuziuk
Copy link
Member

ibuziuk commented Jun 3, 2022

@tolusha maybe to avoid confusion lets merge #1324 first ?
Basically, the resulting definition should be smth like this:

devEnvironments:
  (...)
  defaultContainer: |
    name: universal-developer-image
    image: quay.io/devfile/universal-developer-image:ubi8-latest
    memoryRequest: 256M
    memoryLimit: 1536M
    cpuRequest: 0.1
    cpuLimit: 0.5
  defaultEditor: eclipse/che-theia/next

@tolusha
Copy link
Contributor

tolusha commented Jun 3, 2022

Agree. let's wait for #1324

Copy link
Contributor

@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.

Generally LGTM, but could use some improved docs. I also wonder how this will interplay with #1324, I suspect it'll cause merge conflicts.

Edit: should have read the existing discussion -- apologies there.

api/v1/checluster_types.go Outdated Show resolved Hide resolved
api/v1/checluster_types.go Outdated Show resolved Hide resolved
api/v1/checluster_types.go Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

At this point it will also be necessary to add these fields to the v2 spec and implement conversion between the two -- I can help with that if you like.

Copy link
Contributor

@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 👍

Thanks @olexii4

api/v2/checluster_types.go Show resolved Hide resolved
@openshift-ci openshift-ci bot added the lgtm label Jun 17, 2022
api/v2/checluster_types.go Outdated Show resolved Hide resolved
api/v2/checluster_types.go Outdated Show resolved Hide resolved
api/v2/checluster_types.go Outdated Show resolved Hide resolved
Comment on lines +80 to +92
DefaultEditor string `json:"defaultEditor,omitempty"`
// Default components applied to DevWorkspaces.
// These default components are meant to be used when a Devfile, that does not contain any components.
// +optional
DefaultComponents []devfile.Component `json:"defaultComponents,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

@l0rd please, take a look at the final structure we are planning to have in place:

devEnvironments:
  (...)
  defaultComponents [ ] 
  defaultEditor: ""

Any concerns?

Copy link
Member

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

@olexii4 LGTM
could you please update the description with the new v2 format. Currently it still shows an old CR format with obsolete server instead of devEnvironments

@openshift-ci
Copy link

openshift-ci bot commented Jun 20, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, ibuziuk, olexii4

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot removed the lgtm label Jun 20, 2022
@openshift-ci
Copy link

openshift-ci bot commented Jun 20, 2022

New changes are detected. LGTM label has been removed.

@tolusha
Copy link
Contributor

tolusha commented Jun 20, 2022

[10:22:59] → CustomResourceDefinition.apiextensions.k8s.io "checlusters.org.eclipse.che" is invalid: [spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[container].properties[endpoints].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[container].properties[endpoints].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[kubernetes].properties[endpoints].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[kubernetes].properties[endpoints].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[openshift].properties[endpoints].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[openshift].properties[endpoints].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[plugin].properties[commands].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[plugin].properties[commands].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[plugin].properties[components].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[plugin].properties[components].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[plugin].properties[components].items.properties[container].properties[endpoints].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[plugin].properties[components].items.properties[container].properties[endpoints].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[plugin].properties[components].items.properties[kubernetes].properties[endpoints].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[plugin].properties[components].items.properties[kubernetes].properties[endpoints].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[plugin].properties[components].items.properties[openshift].properties[endpoints].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[0].schema.openAPIV3Schema.properties[spec].properties[server].properties[workspaceDefaultComponents].items.properties[plugin].properties[components].items.properties[openshift].properties[endpoints].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[container].properties[endpoints].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[container].properties[endpoints].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[kubernetes].properties[endpoints].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[kubernetes].properties[endpoints].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[openshift].properties[endpoints].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[openshift].properties[endpoints].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[plugin].properties[commands].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[plugin].properties[commands].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[plugin].properties[components].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[plugin].properties[components].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[plugin].properties[components].items.properties[container].properties[endpoints].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[plugin].properties[components].items.properties[container].properties[endpoints].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[plugin].properties[components].items.properties[kubernetes].properties[endpoints].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[plugin].properties[components].items.properties[kubernetes].properties[endpoints].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[plugin].properties[components].items.properties[openshift].properties[endpoints].items.properties[attributes].allOf[0].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural, spec.versions[1].schema.openAPIV3Schema.properties[spec].properties[devEnvironments].properties[defaultComponents].items.properties[plugin].properties[components].items.properties[openshift].properties[endpoints].items.properties[attributes].allOf[1].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural]

@tolusha
Copy link
Contributor

tolusha commented Jun 20, 2022

@amisevsk Did you face such kind the problem?

@amisevsk
Copy link
Contributor

@tolusha Hmmm, I remember there being some issues with preserveUnknownFields in the devfile/api but I don't remember what was done to fix it.

@tolusha
Copy link
Contributor

tolusha commented Jun 22, 2022

I see difference in CRD generation

devfile:

                                  attributes:
                                    description: "Map of implementation-dependant
                                      string-based free-form attributes. \n Examples
                                      of Che-specific attributes: \n - cookiesAuthEnabled:
                                      \"true\" / \"false\", \n - type: \"terminal\"
                                      / \"ide\" / \"ide-dev\","
                                    type: object
                                    x-kubernetes-preserve-unknown-fields: true

operator:

                                attributes:
                                    additionalProperties:
                                      x-kubernetes-preserve-unknown-fields: true
                                    allOf:
                                    - x-kubernetes-preserve-unknown-fields: true
                                    - x-kubernetes-preserve-unknown-fields: true
                                    description: "Map of implementation-dependant
                                      string-based free-form attributes. \n Examples
                                      of Che-specific attributes: \n - cookiesAuthEnabled:
                                      \"true\" / \"false\", \n - type: \"terminal\"
                                      / \"ide\" / \"ide-dev\","
                                    type: object

olexii4 and others added 5 commits June 22, 2022 14:25
Signed-off-by: Oleksii Orel <oorel@redhat.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
@tolusha
Copy link
Contributor

tolusha commented Jun 22, 2022

Switched to a new controller-gen version (v0.4.1 -> v0.7.0)
It seems v0.4.1 has a bug

Signed-off-by: Anatolii Bazko <abazko@redhat.com>
@tolusha
Copy link
Contributor

tolusha commented Jun 22, 2022

/retest

@openshift-ci
Copy link

openshift-ci bot commented Jun 22, 2022

@olexii4: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v9-devworkspace-happy-path 5a051a3 link true /test v9-devworkspace-happy-path
ci/prow/v8-operator-test 5a051a3 link true /test v8-operator-test

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@tolusha tolusha merged commit 3830ac4 into main Jun 22, 2022
@tolusha tolusha deleted the CHE-21353 branch June 22, 2022 13:52
@che-bot che-bot added this to the 7.50 milestone Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants