capitalizeMsg function to correctly capitalize the first character an… #1276

Horiodino · 2024-06-21T11:19:53Z

handle error msf scenarios based

What does this PR do?

What issues does this PR fix or reference?

#1092

Is it tested? How?

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

openshift-ci · 2024-06-21T11:20:06Z

Hi @Horiodino. Thanks for your PR.

I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

Horiodino · 2024-06-21T11:22:52Z

i was thinking in the dw structure we can add a context of type error if its for user we can set it as true and then at the end we can dw is for error handling and representing we can directly add function for that.
but for that we need to set context like if the error is for user then we need to set it as true and after that we can again set it to false ?

AObuchow

Thank you for your PR @Horiodino :)
It seems you're missing testing instructions. Could you please add some?
Have you tried building DevWorkspace Operator with your changes and deploying it to a testing cluster (like minikube for instance?)

I took a look through the codebase to try and find a user-facing error message that is not already being capitalized, and is simple enough to trigger. What I found was that finalize function (which gets called when a workspace is being terminated/deleted off the cluster) will try and check the workspace's storageProvisioner (i.e. storage type). The potential error that could happen at this step is not being capitalized.

You should be able to trigger this (normally not-capitalized error) by:

Creating a workspace with a valid storage strategy attribute set, e.g.:

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: plain-devworkspace
spec:
  started: true
  routingClass: 'basic'
  template:
    attributes:
      controller.devfile.io/storage-type: per-user
    components:
      - name: web-terminal
        container:
          image: quay.io/wto/web-terminal-tooling:next
          memoryRequest: 256Mi
          memoryLimit: 512Mi
          mountSources: true
          command:
           - "tail"
           - "-f"
           - "/dev/null"

Let the workspace startup successfully
Modify the workspace's storage-type attribute to an invalid value
Delete the workspace object off the cluster, i.e. kubectl delete dw plain-devworkspace -n <your-workspace's-namespace>
Check that the workspace status error message is capitalized by checking the devworkspace object or doing a kubectl get dw -n <your-workspace's-namespace>

controllers/workspace/condition.go

AObuchow · 2024-06-24T22:42:01Z

i was thinking in the dw structure we can add a context of type error if its for user we can set it as true and then at the end we can dw is for error handling and representing we can directly add function for that. but for that we need to set context like if the error is for user then we need to set it as true and after that we can again set it to false ?

Anytime we're setting the workspace status, such as here, it is already implied that it is a user-facing message. As the workspace status will be provided in the workspace object on the cluster. So there should be no need for this additional context IMO.

Horiodino · 2024-07-05T13:09:56Z

sorry for making so long , the make docker was taking so much time (might be isp problem) .
on running

kubectl patch dw plain-devworkspace --type=merge -p '{"spec":{"template":{"attributes":{"controller.devfile.io/storage-type": "invalid-type"}}}}'

kubectl get dw -w
NAME                 DEVWORKSPACE ID             PHASE     INFO
plain-devworkspace   workspace147061c666194f78   Running   Workspace is running
plain-devworkspace   workspace147061c666194f78   Running   Workspace is running
plain-devworkspace   workspace147061c666194f78   Failing   Error Provisioning Storage: Configured Storage Type Not Supported
plain-devworkspace   workspace147061c666194f78   Failing   Error Provisioning Storage: Configured Storage Type Not Supported
plain-devworkspace   workspace147061c666194f78   Failing   Error Provisioning Storage: Configured Storage Type Not Supported
plain-devworkspace   workspace147061c666194f78   Failing   Error Provisioning Storage: Configured Storage Type Not Supported
plain-devworkspace   workspace147061c666194f78   Failed    Error Provisioning Storage: Configured Storage Type Not Supported

Horiodino · 2024-07-05T13:10:57Z

/test v8-devworkspace-operator-e2e, v8-che-happy-path

openshift-ci · 2024-07-05T13:11:13Z

@Horiodino: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test v8-devworkspace-operator-e2e, v8-che-happy-path

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-sigs/prow repository.

AObuchow · 2024-07-05T17:01:59Z

@Horiodino thank you for the follow-up. Could you please run a go mod tidy on your fork and commit the changes? This should fix the failing GH Action.

You might also need to do a go install golang.org/x/tools/cmd/goimports@latest followed by a make fmt

Horiodino · 2024-07-09T11:39:28Z

@AObuchow

AObuchow · 2024-07-13T23:06:03Z

@Horiodino sorry for the delay, I'm planning to continue reviewing your PR early this week.

AObuchow

Sorry for the delay in following up on this @Horiodino.

I made a silly mistake in my original review: cases.Title(language.English, cases.NoLower) actually sets every word in a string to start with an upper case character, which is not what we want here.

You can remove commits f56a79c & c431b0e from your PR and just keep e758926. I'll add some extra comments on that commit.

Also for testing, you should be running

kubectl patch dw plain-devworkspace -n $NAMESPACE --type=merge -p '{"spec":{"template":{"attributes":{"controller.devfile.io/storage-type": "invalid-type"}}}}' && kubectl delete dw plain-devworkspace -n $NAMESPACE

as the workspace needs to be deleted immediately after setting an invalid storage type.

The output of kubectl get dw -n $NAMESPACE -w should include something like:

plain-devworkspace   workspace3a2024c23f13435f   Error         Configured Storage Type Not Supported

AObuchow · 2024-07-15T20:34:24Z

controllers/workspace/condition.go

@@ -130,3 +132,12 @@ func getConditionIndexInOrder(condType dw.DevWorkspaceConditionType) int {
 	}
 	return -1
 }
+
+func capitalizeMsg(msg string) string {
+	if len(msg) == 0 {


We usually check for empty strings directly, rather than checking the length of the string. So this can be rewritten as:

if msg == "" { return msg }

AObuchow · 2024-07-15T20:38:16Z

controllers/workspace/condition.go

@@ -130,3 +132,12 @@ func getConditionIndexInOrder(condType dw.DevWorkspaceConditionType) int {
 	}
 	return -1
 }
+
+func capitalizeMsg(msg string) string {


I think this function name could be unabbreviated as capitalizeMessage(msg string).

Also, it's worth adding a simple function doc, something like:
// Capitalizes the first character in the given string.

Horiodino · 2024-07-17T07:18:04Z

working as expected
plain-devworkspace workspaced049889b5c554e91 Error Configured storage type not supported

AObuchow · 2024-07-17T16:23:48Z

/ok-to-test

AObuchow

LGTM. @dkwon17 any thoughts? I'll wait for you to give this a look before we merge.

dkwon17

Sorry, just getting to this now,

LGTM, let's have it for DWO 0.31.x

openshift-ci · 2024-08-07T21:15:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AObuchow, dkwon17, Horiodino

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~~ [AObuchow,dkwon17]

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

AObuchow · 2024-08-13T21:10:54Z

@Horiodino This looks just about good to merge finally. @Horiodino could you please squashing both of your commits and perhaps rewording the commit message to "feat: ensure all DevWorkspace condition messages are capitalized" (or something similar)?

Thank you :)

openshift-ci · 2024-08-17T15:46:19Z

New changes are detected. LGTM label has been removed.

AObuchow · 2024-08-20T15:24:34Z

@Horiodino Sorry for being so picky, but your commit now has the author sign-off twice in the description, and it's also lacking a reference to the bug it's resolving. Could you please modify your commit message to:

feat: ensure all DevWorkspace condition messages are capitalized

Fix #1092 

Signed-off-by: Horiodino <holiodin@gmail.com>

Fix devfile#1092 Signed-off-by: Horiodino <holiodin@gmail.com>

Horiodino · 2024-08-23T17:00:46Z

@Horiodino Sorry for being so picky, but your commit now has the author sign-off twice in the description, and it's also lacking a reference to the bug it's resolving. Could you please modify your commit message to:
feat: ensure all DevWorkspace condition messages are capitalized

Fix #1092 

Signed-off-by: Horiodino <holiodin@gmail.com>

np , done 👍

Horiodino requested review from AObuchow, dkwon17 and ibuziuk as code owners June 21, 2024 11:19

openshift-ci bot added the needs-ok-to-test label Jun 21, 2024

AObuchow reviewed Jun 24, 2024

View reviewed changes

controllers/workspace/condition.go Outdated Show resolved Hide resolved

Horiodino force-pushed the error-capitalization branch from 41742c9 to f56a79c Compare July 5, 2024 12:38

Horiodino requested a review from AObuchow July 13, 2024 07:07

AObuchow requested changes Jul 15, 2024

View reviewed changes

openshift-ci bot assigned AObuchow Jul 15, 2024

AObuchow reviewed Jul 15, 2024

View reviewed changes

Horiodino force-pushed the error-capitalization branch from b49342d to e70748d Compare July 17, 2024 07:17

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Jul 17, 2024

AObuchow approved these changes Jul 17, 2024

View reviewed changes

openshift-ci bot added lgtm approved labels Jul 17, 2024

dkwon17 approved these changes Aug 7, 2024

View reviewed changes

openshift-ci bot assigned dkwon17 Aug 7, 2024

Horiodino force-pushed the error-capitalization branch from e70748d to 8e304e2 Compare August 17, 2024 15:46

openshift-ci bot removed the lgtm label Aug 17, 2024

Horiodino requested review from AObuchow and dkwon17 August 18, 2024 12:57

feat: ensure all DevWorkspace condition messages are capitalized

76b66f3

Fix devfile#1092 Signed-off-by: Horiodino <holiodin@gmail.com>

Horiodino force-pushed the error-capitalization branch from 8e304e2 to 76b66f3 Compare August 23, 2024 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

capitalizeMsg function to correctly capitalize the first character an… #1276

capitalizeMsg function to correctly capitalize the first character an… #1276

Horiodino commented Jun 21, 2024 •

edited

Loading

openshift-ci bot commented Jun 21, 2024

Horiodino commented Jun 21, 2024

AObuchow left a comment

AObuchow commented Jun 24, 2024

Horiodino commented Jul 5, 2024 •

edited

Loading

Horiodino commented Jul 5, 2024

openshift-ci bot commented Jul 5, 2024

AObuchow commented Jul 5, 2024

Horiodino commented Jul 9, 2024

AObuchow commented Jul 13, 2024

AObuchow left a comment

AObuchow Jul 15, 2024

AObuchow Jul 15, 2024

Horiodino commented Jul 17, 2024

AObuchow commented Jul 17, 2024

AObuchow left a comment

dkwon17 left a comment

openshift-ci bot commented Aug 7, 2024

AObuchow commented Aug 13, 2024

openshift-ci bot commented Aug 17, 2024

AObuchow commented Aug 20, 2024

Horiodino commented Aug 23, 2024

capitalizeMsg function to correctly capitalize the first character an… #1276

Are you sure you want to change the base?

capitalizeMsg function to correctly capitalize the first character an… #1276

Conversation

Horiodino commented Jun 21, 2024 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

openshift-ci bot commented Jun 21, 2024

Horiodino commented Jun 21, 2024

AObuchow left a comment

Choose a reason for hiding this comment

AObuchow commented Jun 24, 2024

Horiodino commented Jul 5, 2024 • edited Loading

Horiodino commented Jul 5, 2024

openshift-ci bot commented Jul 5, 2024

AObuchow commented Jul 5, 2024

Horiodino commented Jul 9, 2024

AObuchow commented Jul 13, 2024

AObuchow left a comment

Choose a reason for hiding this comment

AObuchow Jul 15, 2024

Choose a reason for hiding this comment

AObuchow Jul 15, 2024

Choose a reason for hiding this comment

Horiodino commented Jul 17, 2024

AObuchow commented Jul 17, 2024

AObuchow left a comment

Choose a reason for hiding this comment

dkwon17 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 7, 2024

AObuchow commented Aug 13, 2024

openshift-ci bot commented Aug 17, 2024

AObuchow commented Aug 20, 2024

Horiodino commented Aug 23, 2024

Horiodino commented Jun 21, 2024 •

edited

Loading

Horiodino commented Jul 5, 2024 •

edited

Loading