Update devfile/api dependency and message with info about failures #270

amisevsk · 2021-02-17T01:48:26Z

What does this PR do?

Update the devfile/api dependency to latest 283b0c54946e9fea9872c25e1e086c303688f0e8, including a fix for the new package path (see: devfile/api#280)

Add support for propagating status info to devworkspace status, reusing the message field, now that ideUrl has been removed from printColumns:

❯ kc get dw
NAME           WORKSPACE ID                PHASE     INFO
theia          workspace5eacae28f63149c4   Running   http://workspace5eacae28f63149c4-theia-3100.192.168.49.2.nip.io
web-terminal   workspace548b4c42f1394436   Failed    Failed to install network objects required for devworkspace

What issues does this PR fix or reference?

Closes #232

Is it tested? How?

Test usual flows, functionality should not be changed except for messages

cc: @metlos I suspect you will also have to update your code to adapt to devfile/api#280

* Update Makefile to point at latest devfile/api commit and support v2 * Update to devfile/api commit f33d2987d137225cd1e8975f6fdfdd2663195a37 * Adapt code to support new module name (.../v2) (see: devfile/api#280) Signed-off-by: Angel Misevski <[email protected]>

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

By default, webhooks get an "unknown" side effects field, which prevents dry-runs on involved resources. Instead we set side-effects to "none" to re-enable dry runs. Note: side-effects in this context refers to changes outside the context of the AdmissionReview that is submitted. Signed-off-by: Angel Misevski <[email protected]>

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

Some CRDs reuse types from devfile/api so they need to be regenerated after updating Signed-off-by: Angel Misevski <[email protected]>

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

sleshchenko

The PR LGTM and improves a lot UX.

I have a proposal for another improvement, maybe we should report the progressing condition into INFO, like the following:

//conditionOrders contains ordered conditions in way the should become ready
// first not ready should be propagated to info
var conditionOrders = map[devworkspace.WorkspaceConditionType]string {
  devworkspace.WorkspaceComponentsReady: "Waiting for components",
  devworkspace.WorkspaceServiceAccountReady: "Waiting for service account",
  devworkspace.WorkspaceRoutingReady: "Waiting for routing",
  //devworkspace.WorkspaceDeploymentReady
  //devworkspace.WorkspaceIDEReady
}

func getInfoMessage(workspace *devworkspace.DevWorkspace, conditions map[devworkspace.WorkspaceConditionType]string) string {
	var failedStartMsg string
	for conditionType, conditionMessage := range conditions {
		switch conditionType {
		// Take error condition message as overriding failed start message
		case devworkspace.WorkspaceError:
			return conditionMessage
		case devworkspace.WorkspaceFailedStart:
			failedStartMsg = conditionMessage
		case devworkspace.WorkspaceReady:
			return workspace.Status.IdeUrl
		}
	}
	if failedStartMsg != "" {
		return failedStartMsg
	}

	for conditionType, progress := range conditionOrders {
		if _, present := conditions[conditionType]; !present {
			return progress
		}
	}

	return workspace.Status.IdeUrl
}

^ it's not tested

sleshchenko · 2021-02-17T08:02:59Z

pkg/library/container/mountSources.go

@@ -43,7 +43,7 @@ func handleMountSources(k8sContainer *corev1.Container, devfileContainer *devwor
 	var sourceMapping string
 	if vm := getProjectsVolumeMount(k8sContainer); vm != nil {
 		// Container already mounts projects volume; need to set env vars according to mountPath
-		// TODO: see issue https://github.com/devfile/api/issues/290
+		// TODO: see issue https://github.com/devfile/api/v2/issues/290


I suspect you used find/replace :-D

😄 I'll admit to a non-judicious use of sed here.

sleshchenko · 2021-02-17T08:23:01Z

controllers/workspace/status.go

+	if failedStartMsg != "" {
+		return failedStartMsg
+	}
+	return workspace.Status.IdeUrl


Should we return IdeURL when it's not ready yet?

That's what we currently do -- personally I like it because I can copy it and refresh a few times to load a workspace, rather than repeatedly using kubectl get, but I'm happy to do it either way (and with your suggestion above of reporting latest condition, it makes sense to not put ideURL until it's started anyways)

Probably it's another issue, but I saw we return outdated URL, after routing class is changed but workspace is stopped.

and with proposal to report the latest condition, I just wondered how we can avoid confusion we got when workspace is started but not started, because Che Routing is not available yet.

There we may also report finalizing a workspace, currently the way is to check removal timestamp )

sleshchenko · 2021-02-17T08:26:30Z

/test v5-devworkspaces-operator-e2e

JPinkney · 2021-02-17T12:44:29Z

pkg/library/storage/commonStorage.go

@@ -18,8 +18,8 @@
 // - Devfile API spec is unclear on how mountSources should be handled -- mountPath is assumed to be /projects
 //   and volume name is assumed to be "projects"
 //   see issues:
-//     - https://github.com/devfile/api/issues/290
-//     - https://github.com/devfile/api/issues/291
+//     - https://github.com/devfile/api/v2/issues/290


I think this might be another find and replace error 😉

openshift-ci-robot · 2021-02-17T12:45:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, JPinkney, 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~~ [JPinkney,amisevsk,sleshchenko]

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

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

openshift-ci-robot · 2021-02-18T00:52:13Z

New changes are detected. LGTM label has been removed.

amisevsk · 2021-02-18T00:53:38Z

Updated the status message to show what the startup process is waiting on, as suggested by Serhii:

amisevsk · 2021-02-18T20:29:18Z

/test v5-devworkspaces-operator-e2e

Update status message to show "Stopping"/"Stopped" when a workspace is stopped, and "Cleaning up resources for deletion" when we're waiting on a finalizer to be cleared. Signed-off-by: Angel Misevski <[email protected]>

amisevsk · 2021-02-19T18:03:29Z

Added support for Stopping/Stopped and also the message Cleaning up resources for deletion when a workspace is deleted and we're finalizing.

amisevsk · 2021-02-19T18:03:37Z

/test v5-devworkspaces-operator-e2e

amisevsk added 5 commits February 16, 2021 20:27

Clean up non-working samples and make sure samples valid

3a713b3

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

Propagate condition message to status.message on failure or error

9312003

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

Remove workarounds in flatten library after updating

cb348fc

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

amisevsk requested review from metlos, sleshchenko and JPinkney February 17, 2021 01:48

openshift-ci-robot added the approved label Feb 17, 2021

amisevsk changed the title ~~Update api add status~~ Update devfile/api dependency and message with info about failures Feb 17, 2021

amisevsk added 2 commits February 16, 2021 20:49

Regenerate CRDs after updating devfile/api

0004ae8

Some CRDs reuse types from devfile/api so they need to be regenerated after updating Signed-off-by: Angel Misevski <[email protected]>

Regenerate default deployment files to reflect updated CRDs

a5e3af7

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

sleshchenko approved these changes Feb 17, 2021

View reviewed changes

openshift-ci-robot assigned sleshchenko Feb 17, 2021

openshift-ci-robot added the lgtm label Feb 17, 2021

JPinkney approved these changes Feb 17, 2021

View reviewed changes

openshift-ci-robot assigned JPinkney Feb 17, 2021

amisevsk added 2 commits February 17, 2021 16:56

fixup: find-replace errors in issue links

d8cdbc7

Rework status message to show current step in workspace startup

5d44ea8

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

openshift-ci-robot removed the lgtm label Feb 18, 2021

Update workspace status message to include stopped and deleted

375efe9

Update status message to show "Stopping"/"Stopped" when a workspace is stopped, and "Cleaning up resources for deletion" when we're waiting on a finalizer to be cleared. Signed-off-by: Angel Misevski <[email protected]>

amisevsk merged commit bd3fcca into devfile:master Feb 22, 2021

amisevsk deleted the update-api-add-status branch February 22, 2021 21:54

This was referenced Feb 22, 2021

Add support for plugins referenced by URI or in external registries #269

Merged

Add support for ephemeral volumes #281

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update devfile/api dependency and message with info about failures #270

Update devfile/api dependency and message with info about failures #270

amisevsk commented Feb 17, 2021

sleshchenko left a comment

sleshchenko Feb 17, 2021

amisevsk Feb 17, 2021

sleshchenko Feb 17, 2021

amisevsk Feb 17, 2021

sleshchenko Feb 17, 2021

sleshchenko commented Feb 17, 2021

JPinkney Feb 17, 2021

openshift-ci-robot commented Feb 17, 2021

openshift-ci-robot commented Feb 18, 2021

amisevsk commented Feb 18, 2021

amisevsk commented Feb 18, 2021

amisevsk commented Feb 19, 2021

amisevsk commented Feb 19, 2021

Update devfile/api dependency and message with info about failures #270

Update devfile/api dependency and message with info about failures #270

Conversation

amisevsk commented Feb 17, 2021

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

sleshchenko left a comment

Choose a reason for hiding this comment

sleshchenko Feb 17, 2021

Choose a reason for hiding this comment

amisevsk Feb 17, 2021

Choose a reason for hiding this comment

sleshchenko Feb 17, 2021

Choose a reason for hiding this comment

amisevsk Feb 17, 2021

Choose a reason for hiding this comment

sleshchenko Feb 17, 2021

Choose a reason for hiding this comment

sleshchenko commented Feb 17, 2021

JPinkney Feb 17, 2021

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 17, 2021

openshift-ci-robot commented Feb 18, 2021

amisevsk commented Feb 18, 2021

amisevsk commented Feb 18, 2021

amisevsk commented Feb 19, 2021

amisevsk commented Feb 19, 2021