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

Refactor machine output code #4948

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Jul 27, 2021

What type of PR is this?

/kind feature

What does this PR do / why we need it:

Refactored the code for the JSON output of the following commands:

  • odo debug info
  • odo env view
  • odo project create
  • odo project delete
  • odo project get
  • odo project list
  • odo storage create
  • odo storage delete
  • odo url create
  • odo url list
  • odo component list

Main changes:

  • project.GetMachineReadableFormat function renamed to project.NewProject (and similar for other kinds)
  • project.getMachineReadableFormatForList renamed to project.NewProjectList (and similar for other kinds)
  • (o Project) HumanReadableOutput created (and similar for other kinds)

Kinds are capitalized:

  • url -> URL
  • storage -> Storage

Messages are not included in the returned value:

There are no message fields in the odo objects (Project, Storage, etc).

$ odo project create test1 -o json
{
"kind": "Project",
"apiVersion": "odo.dev/v1alpha1",
"metadata": {
"name": "test1",
"namespace": "test1",
"creationTimestamp": null
},
"message": "Project 'test1' is ready for use"
}

Delete commands return a Status object:

APIs generally does not return the deleted object. If we want to try to be compatible with other Kubernetes CLIs/APIs, we could use the Status kind returned by calls to the Kubernetes API DELETE endpoints (example: https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/deployment-v1/#delete-delete-a-deployment).

$ odo project delete prj2 -o json
{
	"kind": "Status",
	"apiVersion": "v1",
	"metadata": {},
	"status": "Success",
	"message": "Deleted project : prj2",
	"details": {
		"name": "prj2",
		"kind": "Project"
	}
}

Which issue(s) this PR fixes:

Related to #2784

PR acceptance criteria:

  • Unit test

  • Integration test

    • odo debug info
    • odo env view
    • odo project create
    • odo project delete
    • odo project get
    • odo project list
    • odo storage list
    • odo storage create
    • odo storage delete
    • odo url create
    • odo url list
    • odo component list
  • Documentation

  • Update changelog

  • I have read the test guidelines

How to test changes / Special notes to the reviewer:

@netlify
Copy link

netlify bot commented Jul 27, 2021

✔️ Deploy Preview for odo-docusaurus-preview ready!

🔨 Explore the source changes: 48e1261

🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/612de797c21e620008441c67

😎 Browse the preview: https://deploy-preview-4948--odo-docusaurus-preview.netlify.app

@feloy feloy force-pushed the refacto-2784/json-machine-output branch 2 times, most recently from e10a1ac to 273ef99 Compare July 28, 2021 09:45
@prietyc123
Copy link
Contributor

/test v4.8-integration-e2e

@feloy feloy force-pushed the refacto-2784/json-machine-output branch from 273ef99 to f610a55 Compare July 29, 2021 08:28
@feloy feloy force-pushed the refacto-2784/json-machine-output branch 2 times, most recently from cd8aa02 to 1f9242c Compare July 29, 2021 09:53
@feloy feloy force-pushed the refacto-2784/json-machine-output branch from 1f9242c to 8718326 Compare July 30, 2021 07:56
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 8, 2021
pkg/component/types.go Outdated Show resolved Hide resolved
@feloy feloy force-pushed the refacto-2784/json-machine-output branch from 8718326 to 6ee2bb6 Compare August 16, 2021 07:47
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 16, 2021
@feloy feloy force-pushed the refacto-2784/json-machine-output branch 2 times, most recently from 5fc79f8 to a8b1aaf Compare August 16, 2021 11:40
@feloy feloy force-pushed the refacto-2784/json-machine-output branch from a8b1aaf to a0fd065 Compare August 17, 2021 15:33
@feloy
Copy link
Contributor Author

feloy commented Aug 17, 2021

/test v4.8-integration-e2e

[skipping gathering secrets/support due to error: secrets "support" not found, skipping gathering EgressFirewall.k8s.ovn.org due to error: the server doesn't have a resource type "EgressFirewall", skipping gathering EgressIP.k8s.ovn.org due to error: the server doesn't have a resource type "EgressIP", skipping gathering endpoints/host-etcd-2 due to error: endpoints "host-etcd-2" not found, skipping gathering namespaces/openshift-manila-csi-driver due to error: namespaces "openshift-manila-csi-driver" not found]error: gather did not start for pod must-gather-99rhq: unable to pull image: ImagePullBackOff:

@feloy
Copy link
Contributor Author

feloy commented Aug 18, 2021

/test v4.8-integration-e2e

[Fail] odo devfile registry command tests When executing registry commands with the registry is not present [It] Should successfully add the registry 

@feloy feloy added kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation and removed kind/cleanup labels Aug 18, 2021
@feloy
Copy link
Contributor Author

feloy commented Aug 18, 2021

/test v4.8-integration-e2e

(passes locally)

[Fail] odo project command tests [It] should list current empty project in json format 
/go/src/github.com/openshift/odo/tests/integration/project/cmd_project_test.go:68

@feloy feloy force-pushed the refacto-2784/json-machine-output branch from a0fd065 to 7045a40 Compare August 18, 2021 13:50
Copy link
Member

@dharmit dharmit left a comment

Choose a reason for hiding this comment

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

Only reviewed the documentation changes.

@@ -0,0 +1,43 @@
---
title: JSON Output
sidebar_position: 2
Copy link
Member

Choose a reason for hiding this comment

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

We're planning to put Spring Boot + Postgres in the "Using odo" section such that it'll show the complete flow starting from creating a component to creating URL, storage, services, and linking with services, components, etc. In that context, putting JSON Output doc under "Using odo" seems out of place to me.

For a few guides that we're importing from old website which don't make sense to be put in "Using odo" section, we are putting them under "Tutorials". A PR is open for it #4934.

I'm inclined to put this under "Tutorials" as well. What do you feel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to place this page under a "Reference" section

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that makes a lot of sense. Where do you think we could put up that section? Current sections:

  • Introduction (no sub-sections)
  • Getting Started
  • Using odo
  • Command Reference
  • Tutorials
  • Architecture
  • Contributing to odo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This page could appear at the end of the Command Reference section

Copy link
Member

Choose a reason for hiding this comment

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

Ack. 👍🏾

Copy link
Member

Choose a reason for hiding this comment

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

@feloy can you create a new page and move this to the Command Reference section, or do you intend to do this in another PR?

| odo application list | List (odo.dev/v1alpha1) | Application (odo.dev/v1alpha1) | ? |
| odo catalog list components | List (odo.dev/v1alpha1) | ComponentType (odo.dev/v1alpha1) (s2i) / *missing* (devfile) | yes (s2i) / yes (devfile) |
| odo catalog list services | List (odo.dev/v1alpha1) | ClusterServiceVersion (operators.coreos.com/v1alpha1) | ? |
| odo catalog describe component | *missing* | *n/a* | yes |
Copy link
Member

Choose a reason for hiding this comment

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

How's "missing" different from "n/a"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"missing" indicates that the output does not contain the kind/version information, but should contain it.

| commands | Kind (version) | Kind (version) of list items | Complete content? |
|--------------------------------|-----------------------------------------|--------------------------------------------------------------|---------------------------|
| odo application describe | Application (odo.dev/v1alpha1) | *n/a* |no |
| odo application list | List (odo.dev/v1alpha1) | Application (odo.dev/v1alpha1) | ? |
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between "n/a" and "?" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

n/a (not applicable) indicates that this information (Kind (version) of list items) if not applicable, because the discussed resource is not a list

"?" is a request for comment from the team, to discuss if the content is useful and complete, or if we should work on it

website/docs/using-odo/json-output.md Outdated Show resolved Hide resolved

The exhaustive list of commands accepting the `-o json` flag is currently:

| commands | Kind (version) | Kind (version) of list items | Complete content? |
Copy link
Member

Choose a reason for hiding this comment

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

What does "Complete content" mean? Are there any commands where it's incomplete? I'd expect this to be binary - either JSON output is present or not present. But "Complete" makes me think it's about something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the content contains all the data present in the human readale output (or more), and is equivalent to it.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 31, 2021
@feloy feloy moved this from For Review to Done in Sprint 206 Aug 31, 2021
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

6 similar comments
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@feloy feloy moved this from Done to For Review in Sprint 206 Sep 2, 2021
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@feloy feloy force-pushed the refacto-2784/json-machine-output branch from 48e1261 to 584b34a Compare September 2, 2021 11:03
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 2, 2021
@sonarcloud
Copy link

sonarcloud bot commented Sep 2, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
4.1% 4.1% Duplication

@feloy
Copy link
Contributor Author

feloy commented Sep 2, 2021

psi-kubernetes-integration-e2e:

[ssh:Fedora-32-minikube]         mkdir /tmp/475500516: no space left on device

@feloy
Copy link
Contributor Author

feloy commented Sep 2, 2021

/test psi-kubernetes-integration-e2e

@openshift-ci
Copy link

openshift-ci bot commented Sep 2, 2021

@feloy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
ci/prow/v4.7-integration-e2e e10a1ac link /test v4.7-integration-e2e

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@dharmit dharmit removed this from For Review in Sprint 206 Sep 2, 2021
@valaparthvi
Copy link
Member

/lgtm

Adding again; removed due to rebase.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 2, 2021
@feloy
Copy link
Contributor Author

feloy commented Sep 2, 2021

/test psi-kubernetes-integration-e2e

@openshift-merge-robot openshift-merge-robot merged commit 0e68584 into redhat-developer:main Sep 2, 2021
@kadel kadel mentioned this pull request Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants