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

Add support for contributions through .spec.contributions field in DevWorkspace #939

Merged
merged 5 commits into from
Oct 18, 2022

Conversation

amisevsk
Copy link
Collaborator

What does this PR do?

Adds support for the .spec.contributions field as described in devfile/api#874. The new field functions the same as current plugin components but can be defined separately from .spec.template, allowing easier conversion from devfile to devworkspace and back.

For test cases, I copied most of the existing tests for plugins functionality and adapted them to use contributions instead.

What issues does this PR fix or reference?

Part of #656

Is it tested? How?

This PR adds a new sample: samples/theia-next-contributions.yaml that uses the new field instead of a theia plugin component. This should behave identically to the current theia-next.yaml sample.

In addition I've rewritten the samples from #844 to use contributions instead of plugin components:

  • checode + golang [link]:

    kubectl apply -f https://gist.githubusercontent.com/amisevsk/b44ab9d83c2ac6cb4c500ad16ee70ca7/raw/a4506801c3d20e6a1423a98238d7e8d9636005f3/code-golang.devfile.yaml
    
  • theia + golang [link]:

    kubectl apply -f https://gist.githubusercontent.com/amisevsk/b44ab9d83c2ac6cb4c500ad16ee70ca7/raw/a4506801c3d20e6a1423a98238d7e8d9636005f3/theia-golang.devworkspace.yaml
    

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
    • v8-devworkspace-operator-e2e: DevWorkspace e2e test
    • v8-che-happy-path: Happy path for verification integration with Che

@amisevsk
Copy link
Collaborator Author

/retest

@AObuchow
Copy link
Collaborator

AObuchow commented Oct 4, 2022

@amisevsk the code looks good to me, though in my testing on Minikube I found that the devworkspace URL does not appear with the theia-next-contributions.yaml sample or checode + golang.

[aobuchow@fedora samples]$ kubectl get dw -w
NAME                DEVWORKSPACE ID             PHASE      INFO
test-contrib-code   workspace30e25356b27043bc   Starting   Waiting for workspace deployment
test-contrib-code   workspace30e25356b27043bc   Starting   Waiting for workspace deployment
test-contrib-code   workspace30e25356b27043bc   Running    Workspace is running

Is there something I'm missing here?

@amisevsk
Copy link
Collaborator Author

amisevsk commented Oct 4, 2022

@AObuchow Weird, that doesn't reproduce for me. Are you sure you're running the changes from this PR? New CRDs + old DWO image would explain that.

@AObuchow
Copy link
Collaborator

AObuchow commented Oct 4, 2022

@AObuchow Weird, that doesn't reproduce for me. Are you sure you're running the changes from this PR? New CRDs + old DWO image would explain that.

I deleted my minikube cluster and reinstalled everything, just to make sure I had a clean slate.

I was able to get the theia-next-contributions.yaml sample working by building and deploying the DWO controller image to minikube (i.e. not scaling the deployment to 0) rather than running the controller locally. However, when running the controller locally, this issue still occurs.

I checked the git log and diff'ed the CRD's and code changes and everything seems in place.

Are you testing with the controller running locally as well? If so, then it's something else on my end for sure.

@amisevsk
Copy link
Collaborator Author

amisevsk commented Oct 5, 2022

@AObuchow doesn't reproduce when running locally either. Try a clean install: make uninstall, make install

@dkwon17
Copy link
Collaborator

dkwon17 commented Oct 5, 2022

I've only tested locally, and got the same behaviour as @AObuchow if I apply the template with oc apply -f samples/theia-next-contributions.yaml. For some reason, the spec.contributions field is removed when creating the DevWorkspace.

However, I'm able to do oc edit devworkspace theia-next and add the spec.contributions back manually, and that got it working; the workspace pod starts with the theia-ide container contributed from spec.contributions

@amisevsk
Copy link
Collaborator Author

amisevsk commented Oct 5, 2022

@dkwon17 does this still occur if you run make uninstall before trying to test?

@dkwon17
Copy link
Collaborator

dkwon17 commented Oct 6, 2022

@amisevsk yes, it still happens if I run:

$ export NAMESPACE="devworkspace-controller"
$ make uninstall
$ make install
$ oc patch deployment/devworkspace-controller-manager --patch "{\"spec\":{\"replicas\":0}}" -n $NAMESPACE
$ make run

and then run:

$ oc project devworkspace-controller
$ oc apply -f samples/theia-next-contributions.yaml

After oc apply, I see the spec.contributions in the kubectl.kubernetes.io/last-applied-configuration annotation, but it's missing in the resource itself unless I manually add it myself

@amisevsk
Copy link
Collaborator Author

amisevsk commented Oct 6, 2022

I've tested on OpenShift 4.11 and minikube (k8s 1.23.3) and could not reproduce this issue 🤔

Tested also on latest minikube (v1.27.0, k8s v1.25.0) and also could not reproduce. Any further insight here? Some options for further debugging when it's reproducible:

  1. Enable the enableExperimentalFeatures option in the DWOC to enable diff logging -- maybe DWO is deleting the components field at some point?
  2. Check the .metadata.managedFields field on DevWorkspaces (kubectl get dw <name> -o yaml --show-managed-fields) to see if something else is setting .spec.contributions

In either case, make sure that the webhooks deployment for your test environment is running a PR branch build -- if your webhooks server is using the old DevWorkspace definitions, it's likely not deserializing the components field to begin with, causing it to be dropped when the mutating webhook adds a creator label.

@dkwon17
Copy link
Collaborator

dkwon17 commented Oct 7, 2022

In either case, make sure that the webhooks deployment for your test environment is running a PR branch build -- if your webhooks server is using the old DevWorkspace definitions, it's likely not deserializing the components field to begin with, causing it to be dropped when the mutating webhook adds a creator label.

Thanks @amisevsk , I realized this was the culprit. If I update the webhook-server image from the devworkspace-webhook-server deployment with the image for this PR, then I don't get the issue of having the spec.contributions disappearing

@amisevsk
Copy link
Collaborator Author

amisevsk commented Oct 7, 2022

Thank goodness, I'm glad we figured it out -- I thought I was losing my mind! 😄

Copy link
Collaborator

@dkwon17 dkwon17 left a comment

Choose a reason for hiding this comment

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

Just an optional suggestion, LGTM

annotate.AddSourceAttributesForTemplate(contribution.Name, resolvedPlugin)
pluginSpecContents = append(pluginSpecContents, &resolvedPlugin.DevWorkspaceTemplateSpecContent)
}
if workspace != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know the best practice for Go, but a small optional suggestion to put the initialization for resolvedParent and resolvedContent here, so that it looks like:

	resolvedParent := &dw.DevWorkspaceTemplateSpecContent{}
	resolvedContent := &dw.DevWorkspaceTemplateSpecContent{}

	if workspace != nil {
		resolvedContent = workspace.DevWorkspaceTemplateSpecContent.DeepCopy()
		resolvedContent.Components = nil
	 ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense, thanks. Squashed change into earlier commit.

@openshift-ci
Copy link

openshift-ci bot commented Oct 13, 2022

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Update devfile/api commit used for Go dependency and CRDs to
devfile/api@fe7c10e

Signed-off-by: Angel Misevski <[email protected]>
…endency

Regenerate files to pull in changes from devfile/api@fe7c10e

Signed-off-by: Angel Misevski <[email protected]>
Add support for specifying additional components in the
`.spec.contributions` field of the DevWorkspace. These
components are defined and handled similarly to plugin components.

Signed-off-by: Angel Misevski <[email protected]>
Add tests covering .spec.contributions field use in DevWorkspaces. This
is done by copying tests for plugin components and adapting them to use
contributions instead to make sure we cover as many use cases as
possible.

Signed-off-by: Angel Misevski <[email protected]>
@amisevsk amisevsk force-pushed the container-contributions-field branch from af200e8 to 9dc8ca3 Compare October 13, 2022 18:25
@openshift-ci openshift-ci bot removed the lgtm label Oct 13, 2022
@openshift-ci
Copy link

openshift-ci bot commented Oct 13, 2022

New changes are detected. LGTM label has been removed.

@codecov
Copy link

codecov bot commented Oct 13, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@eab583f). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #939   +/-   ##
=======================================
  Coverage        ?   48.81%           
=======================================
  Files           ?       36           
  Lines           ?     2325           
  Branches        ?        0           
=======================================
  Hits            ?     1135           
  Misses          ?     1077           
  Partials        ?      113           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@amisevsk amisevsk merged commit cb44096 into devfile:main Oct 18, 2022
@amisevsk amisevsk deleted the container-contributions-field branch October 18, 2022 16:27
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