Implement API endpoints by feloy · Pull Request #6915 · redhat-developer/odo

feloy · 2023-06-19T14:57:36Z

What type of PR is this:

/kind feature
What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #6302

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:

netlify · 2023-06-19T14:57:40Z

✅ Deploy Preview for odo-docusaurus-preview ready!

Name	Link
🔨 Latest commit	`cbb362d`
🔍 Latest deploy log	https://app.netlify.com/sites/odo-docusaurus-preview/deploys/64998832a822160008ff33e3
😎 Deploy Preview	https://deploy-preview-6915--odo-docusaurus-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

odo-robot · 2023-06-19T15:04:42Z

OpenShift Unauthenticated Tests on commit e30e203 finished successfully.
View logs: TXT HTML

odo-robot · 2023-06-19T15:06:01Z

NoCluster Tests on commit e30e203 finished successfully.
View logs: TXT HTML

odo-robot · 2023-06-19T15:07:28Z

Unit Tests on commit e30e203 finished successfully.
View logs: TXT HTML

odo-robot · 2023-06-19T15:12:26Z

Validate Tests on commit e30e203 finished successfully.
View logs: TXT HTML

odo-robot · 2023-06-19T16:06:07Z

Windows Tests (OCP) on commit e30e203 finished successfully.
View logs: TXT HTML

odo-robot · 2023-06-19T16:41:35Z

Kubernetes Tests on commit e30e203 finished with errors.
View logs: TXT HTML

odo-robot · 2023-06-19T19:53:33Z

OpenShift Tests on commit e30e203 finished successfully.
View logs: TXT HTML

odo-robot · 2023-06-20T10:41:45Z

Kubernetes Docs Tests on commit 4b9c89e finished with errors.
View logs: TXT HTML

valaparthvi

The normal workflow works well for me, but I am unable to run parallel sessions on mac:

$ ODO_EXPERIMENTAL_MODE=t odo dev --api-server
============================================================================
⚠ Experimental mode enabled. Use at your own risk.
More details on https://odo.dev/docs/user-guides/advanced/experimental-mode
============================================================================

  __
 /  \__     Developing using the "my-go-app" Devfile
 \__/  \    Namespace: default
 /  \__/    odo version: v3.11.0
 \__/

 ⚠  You are using "default" namespace, odo may not work as expected in the default namespace.
 ⚠  You may set a new namespace by running `odo create namespace <name>`, or set an existing one by running `odo set namespace <name>`

↪ Running on the cluster in Dev mode
I0621 16:41:04.909656   22390 starterserver.go:58] API Server started at localhost:20000/api/v1
I0621 16:41:04.909831   22390 starterserver.go:78] Stopping the API server; encountered error: listen tcp :20000: bind: address already in use
Cleaning resources, please wait
 ✗  error watching deployment: context canceled

$ ODO_EXPERIMENTAL_MODE=t odo dev --api-server  --platform podman
============================================================================
⚠ Experimental mode enabled. Use at your own risk.
More details on https://odo.dev/docs/user-guides/advanced/experimental-mode
============================================================================

  __
 /  \__     Developing using the "my-go-app" Devfile
 \__/  \    Platform: podman
 /  \__/    odo version: v3.11.0
 \__/

↪ Running on podman in Dev mode
I0621 16:44:09.646006   22887 starterserver.go:58] API Server started at localhost:20000/api/v1
I0621 16:44:09.646188   22887 starterserver.go:78] Stopping the API server; encountered error: listen tcp :20000: bind: address already in use
 ✓  Deploying pod [8s]
 ✗  Syncing files into the container [4ms]
Error occurred on Push - failed to sync to component with name my-go-app: unable to exec command [rm -rf /projects/*]: context canceled


↪ Dev mode
 Status:
 Watching for changes in the current directory /tmp/nodejs-debug

 Keyboard Commands:
[Ctrl+c] - Exit and delete resources from podman
     [p] - Manually apply local changes to the application on podman
Cleaning up resources
 ✗  Dev mode interrupted by user

I think it again has to do with the fact that mac believes a port is free even if it is in use.

Perhaps we can add an --api-address flag in the future iterations.

Also, cluster was very quick in terminating the session, but podman took a while to terminate the session when context was cancelled.

pkg/apiserver-impl/api_default_service.go

pkg/dev/interface.go

tests/integration/cmd_dev_api_server_test.go

pkg/component/describe/describe.go

pkg/apiserver-impl/api_default_service.go

rm3l · 2023-06-22T15:47:26Z

pkg/apiserver-impl/api_default_service.go

+		s.pushWatcher <- struct{}{}
+		return openapi.Response(http.StatusOK, nil), nil
+	default:
+		return openapi.Response(http.StatusBadRequest, nil), fmt.Errorf("command name %q not supported. Supported values are: %q", componentCommandPostRequest.Name, "push")


Wondering if the response body in case of error should not be serialized as a real JSON string too. Currently, I get a simple string, which does not respect the response content type returned in the response headers:

$ http POST http://127.0.0.1:20002/api/v1/component/command name=fake-cmd HTTP/1.1 400 Bad Request Content-Length: 74 Content-Type: application/json; charset=UTF-8 Date: Thu, 22 Jun 2023 15:45:12 GMT "command name \"fake-cmd\" not supported. Supported values are: \"push\""

I'd expect something like below. What do you think?

{ "message": "command name \"fake-cmd\" not supported. Supported values are: \"push\"" }

I think we will need to complete the openapi spec, to define a return code in case of error, a,d to point to the correct GeneralError instead of using the default. WDYT?

Yes, that makes sense.

rm3l · 2023-06-22T16:01:29Z

pkg/apiserver-impl/api_default_service.go

-	return openapi.Response(http.StatusNotImplemented, nil), errors.New("ComponentCommandPost method not implemented")
+	switch componentCommandPostRequest.Name {
+	case "push":
+		s.pushWatcher <- struct{}{}


Also, one thing I just noticed with this: when there is already a 'push' operation in progress (for example, I sent a first POST /component/command request, immediately followed by the same request again), the second request would be blocked waiting for the in-progress push operation to finish (which in my case took a lot of seconds).
Wouldn't it make more sense to return immediately if there is already a 'push' operation?

Yes, that makes sense. In this case, which status code do we want to return? StatusServiceUnavailable?

Maybe a 429 Too Many Requests would be relevant, as the user can retry later when there is no other push in progress?

@feloy Not sure if you saw my previous comment.. What do you think?

I changed from StatusServiceUnavailable to StatusTooManyRequests.

I think we will need to update the openapi spec for the non 2xx responses, but I would prefer we discuss it before to write it. That could be part of another PR?

Yes, sure, that can be a separate PR indeed.

Co-authored-by: Armel Soro <[email protected]>

sonarqubecloud · 2023-06-26T12:46:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.0% Duplication

feloy · 2023-06-26T13:53:46Z

/override Kubernetes-Integration-Tests/Kubernetes-Integration-Tests

 [FAILED] [10.576 seconds]
odo devfile deploy command tests deploying devfile with exec when using devfile where the exec command is bound to fail [It] should print the last 100 lines of the log to the output
/go/odo_1/tests/integration/cmd_devfile_deploy_test.go:648

openshift-ci · 2023-06-26T13:53:50Z

@feloy: Overrode contexts on behalf of feloy: Kubernetes-Integration-Tests/Kubernetes-Integration-Tests

Details

In response to this:

/override Kubernetes-Integration-Tests/Kubernetes-Integration-Tests

[FAILED] [10.576 seconds]
odo devfile deploy command tests deploying devfile with exec when using devfile where the exec command is bound to fail [It] should print the last 100 lines of the log to the output
/go/odo_1/tests/integration/cmd_devfile_deploy_test.go:648

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.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 19, 2023

openshift-ci bot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Jun 19, 2023

openshift-ci bot requested review from anandrkskd and valaparthvi June 19, 2023 14:58

feloy closed this Jun 20, 2023

feloy reopened this Jun 20, 2023

feloy changed the title ~~[wip] Implement API endpoints~~ Implement API endpoints Jun 20, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 20, 2023

valaparthvi suggested changes Jun 21, 2023

View reviewed changes

openshift-ci bot assigned valaparthvi Jun 21, 2023

feloy force-pushed the feature-6302/api-endpoints-impl-main branch from cc6c329 to 42b55b4 Compare June 21, 2023 15:45

feloy closed this Jun 22, 2023

feloy reopened this Jun 22, 2023

feloy closed this Jun 22, 2023

feloy reopened this Jun 22, 2023

rm3l reviewed Jun 22, 2023

View reviewed changes

rm3l added the area/dev Issues or PRs related to `odo dev` label Jun 22, 2023

rm3l reviewed Jun 22, 2023

View reviewed changes

feloy force-pushed the feature-6302/api-endpoints-impl-main branch 2 times, most recently from c2ca26c to 21d656c Compare June 23, 2023 14:33

rm3l approved these changes Jun 23, 2023

View reviewed changes

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 23, 2023

rm3l approved these changes Jun 23, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 23, 2023

feloy and others added 13 commits June 25, 2023 20:31

Pass odo context to api server

952ecbd

Get /instance

f4c44af

DELETE /instance implementation

ef96c2a

Move describe logic to pkg/component/describe

191c782

Get /component implementation

1ef6d22

POST /component/command implementation

23f1d58

Fix example by replacing action with name

6315727

Fix integration test

4c23ffb

Integration tests

961a358

Add comment for PushWatcher

13bc66f

Test DELETE /instance without --no-watch

70bdea2

Apply suggestions from code review

877a685

Co-authored-by: Armel Soro <[email protected]>

Return an error if not ready for push

a8bd866

feloy force-pushed the feature-6302/api-endpoints-impl-main branch from 554d20b to a8bd866 Compare June 25, 2023 18:31

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 25, 2023

feloy closed this Jun 26, 2023

feloy reopened this Jun 26, 2023

Fix windows tests

e031198

feloy closed this Jun 26, 2023

feloy reopened this Jun 26, 2023

Fix tests for Windows

cbb362d

rm3l approved these changes Jun 26, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 26, 2023

openshift-merge-robot merged commit 94e3230 into redhat-developer:main Jun 26, 2023

Conversation

feloy commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for odo-docusaurus-preview ready!

Uh oh!

odo-robot bot commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odo-robot bot commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odo-robot bot commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odo-robot bot commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odo-robot bot commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odo-robot bot commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odo-robot bot commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odo-robot bot commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valaparthvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rm3l Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

feloy Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rm3l Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

rm3l Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

feloy Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rm3l Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

rm3l Jun 23, 2023

Choose a reason for hiding this comment

Uh oh!

feloy Jun 23, 2023

Choose a reason for hiding this comment

Uh oh!

rm3l Jun 23, 2023

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 26, 2023

Uh oh!

feloy commented Jun 26, 2023

Uh oh!

openshift-ci bot commented Jun 26, 2023

Uh oh!

Reviewers

feloy commented Jun 19, 2023 •

edited

Loading

netlify bot commented Jun 19, 2023 •

edited

Loading

odo-robot bot commented Jun 19, 2023 •

edited

Loading

odo-robot bot commented Jun 19, 2023 •

edited

Loading

odo-robot bot commented Jun 19, 2023 •

edited

Loading

odo-robot bot commented Jun 19, 2023 •

edited

Loading

odo-robot bot commented Jun 19, 2023 •

edited

Loading

odo-robot bot commented Jun 19, 2023 •

edited

Loading

odo-robot bot commented Jun 19, 2023 •

edited

Loading

odo-robot bot commented Jun 20, 2023 •

edited

Loading

feloy Jun 22, 2023 •

edited

Loading

feloy Jun 22, 2023 •

edited

Loading