search

devfile / devworkspace-operator

Apache License 2.0
61 stars 55 forks source link

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

Closed Horiodino closed 1 month ago

Horiodino commented 3 months ago

handle error msf scenarios based

What does this PR do?

What issues does this PR fix or reference?

https://github.com/devfile/devworkspace-operator/issues/1092

Is it tested? How?

PR Checklist

openshift-ci[bot] commented 3 months ago

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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
Horiodino commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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

openshift-ci[bot] commented 2 months ago

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

In response to [this](https://github.com/devfile/devworkspace-operator/pull/1276#issuecomment-2210853794): >/test v8-devworkspace-operator-e2e, v8-che-happy-path Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
AObuchow commented 2 months ago

@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 commented 2 months ago

@AObuchow

AObuchow commented 2 months ago

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

Horiodino commented 2 months ago

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

AObuchow commented 2 months ago

/ok-to-test

openshift-ci[bot] commented 1 month ago

[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](https://github.com/devfile/devworkspace-operator/blob/main/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 commented 1 month ago

@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[bot] commented 1 month ago

New changes are detected. LGTM label has been removed.

AObuchow commented 1 month ago

@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>
Horiodino commented 1 month ago

@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 :+1:

AObuchow commented 1 month ago

@Horiodino Thank you! Merging, great work :)