Add command to provide starter project information #3173
Conversation
openshift-ci-robot
added
the
kind/feature
skoh7645
changed the title
|
Hi @skoh7645. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
openshift-ci-robot
added
the
needs-ok-to-test
Codecov Report
@@ Coverage Diff @@
## master #3173 +/- ##
==========================================
+ Coverage 46.26% 46.37% +0.10%
==========================================
Files 112 112
Lines 11214 11233 +19
==========================================
+ Hits 5188 5209 +21
+ Misses 5524 5519 -5
- Partials 502 505 +3
Continue to review full report at Codecov.
|
|
/ok-to-test |
openshift-ci-robot
added
ok-to-test
|
/retest |
|
With experimental mode disabled, I still see a message related to devfile logged on stdout. I think this shouldn't happen: $ echo $ODO_EXPERIMENTAL
false
$ odo preference view
PARAMETER CURRENT_VALUE
UpdateNotification
NamePrefix
Timeout
PushTimeout
Experimental
PushTarget
$ odo catalog describe component nodejs
There are no Odo devfile components with the name "nodejs"
S2I Based Components:
-nodejsWhy doesn't $ odo catalog list components
Odo OpenShift Components:
NAME PROJECT TAGS SUPPORTED
nodejs openshift 10,8,8-RHOAR,latest YES
dotnet openshift 2.0,latest NO
httpd openshift 2.4,latest NO
nginx openshift 1.10,1.12,1.8,latest NO
nodejs openshift 6 NO
perl openshift 5.24,5.26,latest NO
php openshift 7.0,7.1,latest NO
python openshift 2.7,3.5,3.6,latest NO
ruby openshift 2.3,2.4,2.5,latest NO
wildfly openshift 10.0,10.1,11.0,12.0,13.0,8.1,9.0,latest NO
Odo Devfile Components:
NAME DESCRIPTION REGISTRY SUPPORTED
maven Upstream Maven and OpenJDK 11 DefaultDevfileRegistry YES
nodejs Stack with NodeJS 10 DefaultDevfileRegistry YES
openLiberty Open Liberty microservice in Java DefaultDevfileRegistry YES
java-spring-boot Spring Boot® using Java DefaultDevfileRegistry YES
$ odo catalog describe component nodejs [65/1934]
⚠ There are multiple components named "nodejs" in different multiple devfile registries.
Devfile Component(s):
* Registry: CheDevfileRegistry
---
ctx:
fs: null
data:
apiVersion: 1.0.0
metadata:
generateName: nodejs-
projects:
- name: nodejs-web-app
source:
type: git
location: https://github.com/che-samples/web-nodejs-sample.git
components:
- type: chePlugin
id: che-incubator/typescript/latest
memoryLimit: 512Mi
- alias: nodejs
mountSources: true
type: dockerimage
image: quay.io/eclipse/che-nodejs10-ubi:7.12.2
memoryLimit: 512Mi
endpoints:
- name: nodejs
port: 3000
commands:
- actions:
- command: npm install
component: nodejs
type: exec
workdir: ${CHE_PROJECTS_ROOT}/nodejs-web-app/app
name: download dependencies
- actions:
- command: nodemon app.js
component: nodejs
type: exec
workdir: ${CHE_PROJECTS_ROOT}/nodejs-web-app/app
name: run the web app
- actions:
- command: nodemon --inspect app.js
component: nodejs
type: exec
workdir: ${CHE_PROJECTS_ROOT}/nodejs-web-app/app
name: run the web app (debugging enabled)
- actions:
- command: 'node_server_pids=$(pgrep -fx ''.*nodemon (--inspect )?app.js'' | tr
"\\n" " ") && echo "Stopping node server with PIDs: ${node_server_pids}" && kill
-15 ${node_server_pids} &>/dev/null && echo ''Done.'''
component: nodejs
type: exec
name: stop the web app
- actions:
- referenceContent: |
{
"version": "0.2.0",
"configurations": [
{
"type": "node",
"request": "attach",
"name": "Attach to Remote",
"address": "localhost",
"port": 9229,
"localRoot": "${workspaceFolder}",
"remoteRoot": "${workspaceFolder}"
}
]
}
type: vscode-launch
name: Attach remote debugger
* Registry: DefaultDevfileRegistry
---
ctx:
fs: null
data:
apiVersion: 1.0.0
metadata:
generateName: nodejs-
projects:
- name: nodejs-web-app
source:
type: git
location: https://github.com/odo-devfiles/nodejs-ex.git
components:
- alias: runtime
mountSources: true
type: dockerimage
image: quay.io/eclipse/che-nodejs10-ubi:nightly
memoryLimit: 1024Mi
endpoints:
- name: nodejs
port: 3000
commands:
- actions:
- command: npm install
component: runtime
type: exec
workdir: ${CHE_PROJECTS_ROOT}/nodejs-web-app/app
name: devBuild
- actions:
- command: nodemon app.js
component: runtime
type: exec
workdir: ${CHE_PROJECTS_ROOT}/nodejs-web-app/app
name: devRun
S2I Based Components:
-nodejsI'm guessing it's a bug for |
| Context("When executing catalog describe component with a component name with multiple components", func() { | ||
| It("should print multiple devfiles from different registries", func() { | ||
| output := helper.CmdShouldPass("odo", "catalog", "describe", "component", "nodejs") | ||
| helper.MatchAllInOutput(output, []string{"Registry: DefaultDevfileRegistry", "Registry: CheDevfileRegistry"}) |
There was a problem hiding this comment.
As per the spec description multiple devfile should be printed and needs to be verified. For example We should verify at least the key content from the respective registry.
There was a problem hiding this comment.
I've changed this test to check for the locations of the 2 starter projects now
| Context("When executing catalog describe component with a component name with a single component", func() { | ||
| It("should only give information about one component", func() { | ||
| output := helper.CmdShouldPass("odo", "catalog", "describe", "component", "maven") | ||
| helper.MatchAllInOutput(output, []string{"generateName: maven-", "- command: java -jar target/*.jar"}) |
There was a problem hiding this comment.
This command now gives a message if there are no projects in the devfile (in the case of the maven component.
|
Pardon my naive comment, but I don't see what value we're providing to the user here with the proposed implementation of The user story and acceptance criteria in #3005 talk about providing the users with:
This command was not providing much value even in the case of s2i based components. But I'd request to make it helpful in the newer version. I'd request to take a look at how $ odo catalog describe service postgresql-persistent
Name | postgresql-persistent
Bindable | true
Operated by the broker | template-service-broker
Short Description | PostgreSQL database service, with
| persistent storage. For more information
| about using this template, including
| OpenShift considerations, see
| https://github.com/sclorg/postgresql-container/.
| NOTE: Scaling to more than one replica is not
| supported. You must have persistent volumes
| available in your cluster to use this template.
Long Description | This template provides a
| standalone PostgreSQL server
| with a database created.
| The database is stored on
| persistent storage. The
| database name, username,
| and password are chosen via
| parameters when provisioning
| this service.
Versions Available |
Tags | database,postgresql
PLANS
*********************** | *****************************************************
Name | default
----------------- | -----------------
Display Name |
----------------- | -----------------
Short Description | Default plan
----------------- | -----------------
Required Params without a |
default value |
----------------- | -----------------
Required Params with a default | POSTGRESQL_VERSION (default:
value | '9.6'), VOLUME_CAPACITY
| (default: '1Gi'),
| DATABASE_SERVICE_NAME
| (default: 'postgresql'),
| MEMORY_LIMIT (default:
| '512Mi'), POSTGRESQL_DATABASE
| (default: 'sampledb')
----------------- | -----------------
Optional Params | NAMESPACE,
| POSTGRESQL_PASSWORD,
| POSTGRESQL_USER
$ odo catalog describe service dh-hello-world-db-apb
Name | dh-hello-world-db-apb
Bindable | true
Operated by the broker | openshift-automation-service-broker
Short Description | A sample APB which deploys
| Hello World Database
Long Description |
Versions Available | docker.io/centos/postgresql-94-centos7
Tags |
PLANS
*********************** | *****************************************************
Name | default
----------------- | -----------------
Display Name | Default
----------------- | -----------------
Short Description | A sample APB which deploys
| Hello World Database
----------------- | -----------------
Required Params without a |
default value |
----------------- | -----------------
Required Params with a default |
value |
----------------- | -----------------
Optional Params | postgresql_user,
| postgresql_database,
| postgresql_password
| If we can provide more structured information like above example, it would add much more value for the user. The |
| want: []byte{79, 75}, | ||
| }, | ||
| { | ||
| name: "Case 2: Input url is invalid", |
There was a problem hiding this comment.
We can check empty string url, no?
There was a problem hiding this comment.
Thanks! I have added this test case
@dharmit |
|
@dharmit it was suggested by @kadel that we print the whole yaml here, to which others did not disagree with (at the time). However, I do see your point in information being difficult to consume in this format. I thought it would be easiest to print the yaml to we ensure that all available project information would be shown (for example, if another project field gets added to the schema, we wouldn't have to update this code to show the new information). Perhaps this solution would work better if we only printed the project section of the yamls? What do you think? |
But it does so by referring to the enabled registry/registries in the environment, right? In this case, I have two registries enabled and they both provide the $ odo registry list
NAME URL
CheDevfileRegistry https://che-devfile-registry.openshift.io
DefaultDevfileRegistry https://raw.githubusercontent.com/elsony/devfile-registry/master
$ odo catalog list components
...
...
Odo Devfile Components:
NAME DESCRIPTION REGISTRY SUPPORTED
java-spring-boot Spring Boot® using Java DefaultDevfileRegistry YES
nodejs Stack with NodeJS 10 DefaultDevfileRegistry YES
openLiberty Open Liberty microservice in Java DefaultDevfileRegistry YES
maven Upstream Maven and OpenJDK 11 DefaultDevfileRegistry YES
Agreed. I saw that was discussed on #3005 (comment).
Yeah it makes sense. No objections against why you did it. And, I think it was already discussed in the original issue before you opened the PR. Sorry if my comment sounded like this was a bad idea. 😄 ☮️ |
Yes. My bad. I'm late in this discussion.
Exactly. Brief info about the starter projects could probably include the link to the project, its type, link to its devfile. But entire devfile is not so useful, IMO.
This looks way more consumable than printing the entire yaml file. However, I'd not suggest you to make any changes to your PR before the participants on the original discussion think that the points I have raised make sense and are worth addressing. Ping @kadel @johnmcollier @GeekArthur. What do you folks think? |
In a lot of situations, I might want to check how the devfile looks without creating a component.
|
Agreed. Is This is how yaml would look on the CLI: But this is how yaml looks in browser: projects:
- name: nodejs-web-app
source:
type: git
location: https://github.com/che-samples/web-nodejs-sample.gitAnd IIUC, IDEs show the output of such commands in a terminal inside the editor. So the output is going to be similar to what one would see on plain CLI.
My bad. I understand it's difficult decide what to omit from a devfile. But assume a scenario where multiple registries configured and a component is provided by more than one registry; the output of IMO, #3173 (comment) is a pretty good gist of the info we could provide to the user. To @skoh7645's suggestion, I'd request we add link to the full devfile so that user can open that up in browser. Something like: projects:
- name: nodejs-web-app
description: An awesome standalone nodejs web app
source:
type: git
location: https://github.com/che-samples/web-nodejs-sample.git
devfile: https://github.com/elsony/devfile-registry/blob/master/devfiles/nodejs/devfile.yamlI'm not sure if |
Devfile might not be easily accessible by the browser. For example, some repositories will require authentication in the future, and then you would have to also deal with passing auth tokens to the browser. We can start with that what is proposed in #3173 (comment) by @skoh7645 and expand it to full YAML dump in the future only when there is a need for that. |
😞 Makes sense.
+1. |
|
Ok, I can make the changes proposed in #3173 (comment) |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@skoh7645 can you please rebase this PR? I'm removing the "approve" on this PR to avoid tests from getting re-triggered automatically. |
dharmit
removed
the
approved
openshift-ci-robot
added
approved
|
/retest |
|
/lgtm since the bot automatically added approve label and there's no change in code except the rebase. |
|
/lgtm |
openshift-ci-robot
added
the
lgtm
|
/retest Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
What type of PR is this?
/kind feature
What does does this PR do / why we need it:
This PR allows users to get information about components using the
odo catalog describe component [component]command. This is useful as it allows users to get some information about the components they are downloading. This way they can also view all projects available and to make an informed decision if there are multiple starter projects.Which issue(s) this PR fixes:
Fixes #3005
How to test changes / Special notes to the reviewer:
odo catalog describe component [component]devfile.yaml