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

Update Devfile doc for Kubernetes and add quarkus doc #3483

Merged
merged 8 commits into from
Jul 22, 2020

Conversation

adisky
Copy link
Contributor

@adisky adisky commented Jul 3, 2020

What type of PR is this?
/kind documentation
[skip ci]

What does does this PR do / why we need it:
Adds document for quarkus component.
Updates v2 registry, odo catalog list component output
And additions to deploy on kubernetes

Which issue(s) this PR fixes:

#2800

How to test changes / Special notes to the reviewer:

@adisky
Copy link
Contributor Author

adisky commented Jul 3, 2020

@cdrage Added a separate doc, as i was thinking this document is getting quite bigger
https://github.com/openshift/odo/blob/master/docs/public/deploying-a-devfile-using-odo.adoc


In this example we will be deploying a https://github.com/odo-devfiles/quarkus-ex[quarkus component] that uses GraalVM and JDK1.8+.

Before you start make sure you have running openshift/kubernetes cluster and do not forget to login if it is openshift cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems better suited for a Note highlighted section. Also
Before you start, make sure you have a running openshift/kubernetes cluster and do not forget to login, if it is a openshift cluster.

----


Continue devloping your application and just run `odo push`, refresh your browser to view the latest changes.
Copy link
Contributor

@mik-dass mik-dass Jul 7, 2020

Choose a reason for hiding this comment

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

You can now continue developing your application. Just run odo push and refresh your browser to view the latest changes.



Continue devloping your application and just run `odo push`, refresh your browser to view the latest changes.
You can also run `odo watch` to watch changes in the source code, just refreshing the browser will render the source code changes.
Copy link
Contributor

@mik-dass mik-dass Jul 7, 2020

Choose a reason for hiding this comment

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

You can also run odo watch to watch changes in the source code. Just refreshing the browser will render the source code changes.


Continue devloping your application and just run `odo push`, refresh your browser to view the latest changes.
You can also run `odo watch` to watch changes in the source code, just refreshing the browser will render the source code changes.
Run `odo delete` to delete the application from cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

from the cluster

@cdrage
Copy link
Member

cdrage commented Jul 7, 2020

Hey @adisky

I don't believe we need a separate document for this.

I recommend that you modify the file: https://github.com/openshift/odo/blob/master/docs/public/deploying-a-devfile-using-odo.adoc and add it as another example below the other spring boot / nodejs examples.

Please follow the similar "template" as the other examples. Unfortunately the OpenShift docs team is quite specific on the format haha.

@adisky
Copy link
Contributor Author

adisky commented Jul 8, 2020

Hey @adisky

I don't believe we need a separate document for this.

I recommend that you modify the file: https://github.com/openshift/odo/blob/master/docs/public/deploying-a-devfile-using-odo.adoc and add it as another example below the other spring boot / nodejs examples.

Please follow the similar "template" as the other examples. Unfortunately the OpenShift docs team is quite specific on the format haha.

@cdrage template here you mean, the steps needs to be the same?

Aditi Sharma added 4 commits July 8, 2020 14:29
Add starting doc for quarkus app
update the doc with odo watch
update quarkus doc with formatting changes.
@adisky adisky force-pushed the quarkus-doc branch 2 times, most recently from fd7ab47 to 1cbccb2 Compare July 8, 2020 12:06
Add quarkus doc
Update for kubernetes
update new v2 registry
update output of odo catalog list components
@adisky adisky changed the title Add Document for quarkus application deployment on odo Update Devfile doc for Kubernetes and add quarkus doc Jul 8, 2020
@@ -133,15 +125,28 @@ Alternatively, you can pass in `--starter` to `odo create` to have odo download

. Create a URL in order to access the deployed component:
+
* Run below for Kubernetes cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe simply For Kubernetes cluster?

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I don't think this matters much... We could actually keep this the same and just remove --host as by default it will use the default hostname? Or add a small note saying: If using Kubernetes, you have to pass in --host


* Enable experimental mode for odo. This can be done by: `odo preference set experimental true`

* Before proceeding, you must know your cluster domain name to specify host for ingress.
Copy link
Member

Choose a reason for hiding this comment

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

We should provide link where users can learn how to do that, or what the ingress host is.

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 also think so, tried an exact way to determine cluster domain name, what i understand till now it is much dependent on the way cluster is deployed.
I have also seen this issue to add document details on how to get --host for url #3463

You can also run `odo watch` to watch changes in the source code. Just refreshing the browser will render the source code changes.
+
Run `odo delete` to delete the application from cluster.

== Deploying a Java Spring Boot® component locally to Docker
Copy link
Member

Choose a reason for hiding this comment

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

I would remove this whole section. Docker is not fully supported, and it might just confuse users

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with Tomas ^^ we no longer need Docker

* Enable experimental mode for odo. This can be done by: `odo preference set experimental true`

== Creating a project
Copy link
Member

Choose a reason for hiding this comment

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

Please, do NOT remove this section. We must have a section for creating a project in each .adoc / markdown file that corresponds to a tutorial.

Copy link
Member

Choose a reason for hiding this comment

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

You can simply remove / copy and paste this section after the Prerequisites for OpenShift cluster parts.


* Before proceeding, you must know your ingress domain cluster name. For example: `apps-crc.testing` is the cluster domain name for https://github.com/code-ready/crc[Red Hat CodeReady Containers]
==== Prerequisites for Openshift Cluster
Copy link
Member

Choose a reason for hiding this comment

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

OpenShift

Copy link
Member

Choose a reason for hiding this comment

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

Prerequisites for an OpenShift cluster

✓ New project created and now using project : myproject
----
+
==== Prerequisites for Kubernetes Cluster
Copy link
Member

Choose a reason for hiding this comment

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

Prerequisites for a Kubernetes cluster

@@ -133,15 +125,28 @@ Alternatively, you can pass in `--starter` to `odo create` to have odo download

. Create a URL in order to access the deployed component:
+
* Run below for Kubernetes cluster
Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I don't think this matters much... We could actually keep this the same and just remove --host as by default it will use the default hostname? Or add a small note saying: If using Kubernetes, you have to pass in --host

@@ -197,7 +202,7 @@ NOTE: You must use your cluster host domain name when creating your URL.
✓ Successfully deleted component
----

== Deploying a Node.js® component to an OpenShift cluster
== Deploying a Node.js® component to an OpenShift/Kubernetes cluster
Copy link
Member

Choose a reason for hiding this comment

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

Add a space between /

----
+

NOTE: You must use your cluster host domain name when creating your URL.
Copy link
Member

Choose a reason for hiding this comment

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

See other commments above, perhaps we can change these sections to say: "If deploying to Kubernetes, you must provide a cluster host name with --host" or something like that.

@@ -304,6 +322,101 @@ NOTE: You must use your cluster host domain name when creating your URL.
✓ Successfully deleted component
----

== Deploying a Quarkus Application on Kubernetes/Openshift cluster

In this example we will be deploying a https://github.com/odo-devfiles/quarkus-ex[quarkus component] that uses GraalVM and JDK1.8+.
Copy link
Member

Choose a reason for hiding this comment

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

capitalize Quarkus


In this example we will be deploying a https://github.com/odo-devfiles/quarkus-ex[quarkus component] that uses GraalVM and JDK1.8+.

. Download the example quarkus component.
Copy link
Member

Choose a reason for hiding this comment

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

Remove period at the end of the sentence, capitalize Quarkus

$ git clone https://github.com/odo-devfiles/quarkus-ex && cd quarkus-ex
----

. Create an quarkus odo component
Copy link
Member

Choose a reason for hiding this comment

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

capitalize Quarkus

Copy link
Member

Choose a reason for hiding this comment

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

Should be Create a Quarkus component maybe?

You can also run `odo watch` to watch changes in the source code. Just refreshing the browser will render the source code changes.
+
Run `odo delete` to delete the application from cluster.

== Deploying a Java Spring Boot® component locally to Docker
Copy link
Member

Choose a reason for hiding this comment

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

Agreed with Tomas ^^ we no longer need Docker

@girishramnani
Copy link
Contributor

@adisky is this PR fixing the #2800 issue? or just partially implementing?

Removed docker doc
Addressed review comments.
@adisky
Copy link
Contributor Author

adisky commented Jul 9, 2020

@girishramnani It partially fixes it, It only add docs, for tests i would create a separate PR.

Added description for ingress.
@adisky
Copy link
Contributor Author

adisky commented Jul 14, 2020

@kadel @cdrage Addressed review comments, PTAL

Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

minor changes!


Create a project to keep your source code, tests, and libraries
organized in a separate single unit.
* Creating a project to keep your source code, tests, and libraries organized in a separate single unit.
Copy link
Member

Choose a reason for hiding this comment

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

Create a project

+
Ingress ip is usually the external ip of ingress controller service, for minikube or crc clusters running in a virtual machine you can get it by
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this can be shortened?

IP needs to be capitalized. Same for CRC and Minikube.

I feel as though since people are developing against Kubernetes and OpenShift already, they already know what an Ingress is. So we can remove that part. The part about minikube or crc looks good though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdrage its just one line, just added to redirect on some ingress document as @kadel suggested above

@@ -131,17 +140,17 @@ Alternatively, you can pass in `--starter` to `odo create` to have odo download
README.md devfile.yaml pom.xml src
----

. Create a URL in order to access the deployed component:
. Create an URL in order to access the deployed component:
Copy link
Member

Choose a reason for hiding this comment

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

a URL not an URL

@@ -238,17 +247,17 @@ In this example we will be deploying an https://github.com/odo-devfiles/nodejs-e
Please use odo push command to create the component with source deployed
----

. Create a URL in order to access the deployed component:
. Create an URL in order to access the deployed component:
Copy link
Member

Choose a reason for hiding this comment

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

a URL

✓ URL mynodejs-8080.apps-crc.testing created for component: mynodejs

To apply the URL configuration changes, please use odo push
----
+
NOTE: You must use your cluster host domain name when creating your URL.
NOTE: If deploying on kubernetes, you need to pass ingress domain name via `--host` flag.
Copy link
Member

Choose a reason for hiding this comment

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

Kubernetes needs to be capitalized.

Global preference was successfully updated
----
+
== Deploying a Quarkus Application on OpenShift / Kubernetes cluster
Copy link
Member

Choose a reason for hiding this comment

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

to an OpenShift / Kubernetes cluster

----

. Create a component configuration using the `java-spring-boot` component-type named `mydockerspringboot`:
. Create a Quarkus odo component
Copy link
Member

Choose a reason for hiding this comment

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

random space added to end of sentence?

@@ -349,65 +339,58 @@ Alternatively, you can pass in `--starter` to `odo create` to have odo download
Please use odo push command to create the component with source deployed
----

. Create a URL in order to access the deployed component:
. Create an URL in order to access the deployed component:
Copy link
Member

Choose a reason for hiding this comment

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

a URL

@adisky
Copy link
Contributor Author

adisky commented Jul 16, 2020

@cdrage Thanks for review, comments addressed

@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #3483 into master will decrease coverage by 0.53%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3483      +/-   ##
==========================================
- Coverage   46.45%   45.91%   -0.54%     
==========================================
  Files         112      114       +2     
  Lines       11237    11530     +293     
==========================================
+ Hits         5220     5294      +74     
- Misses       5513     5722     +209     
- Partials      504      514      +10     
Impacted Files Coverage Δ
pkg/devfile/adapters/docker/component/adapter.go 58.13% <0.00%> (-15.88%) ⬇️
pkg/kclient/pods.go 38.13% <0.00%> (-6.42%) ⬇️
pkg/util/util.go 51.98% <0.00%> (-3.37%) ⬇️
pkg/lclient/containers.go 69.47% <0.00%> (-2.95%) ⬇️
pkg/lclient/mock_client.go 14.11% <0.00%> (-0.83%) ⬇️
pkg/catalog/catalog.go 54.77% <0.00%> (-0.60%) ⬇️
pkg/sync/adapter.go 82.35% <0.00%> (-0.53%) ⬇️
pkg/component/component.go 26.19% <0.00%> (-0.21%) ⬇️
pkg/lclient/client.go 0.00% <0.00%> (ø)
pkg/envinfo/envinfo.go 60.00% <0.00%> (ø)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e367c33...fc2ee75. Read the comment docs.

@girishramnani
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jul 22, 2020
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

LGTM!

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 22, 2020
@cdrage
Copy link
Member

cdrage commented Jul 22, 2020

No point having this sit in the merge queue, so going to merge this manually. Thanks @adisky !

@cdrage cdrage merged commit 299097f into redhat-developer:master Jul 22, 2020
@openshift-ci-robot
Copy link
Collaborator

@adisky: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/v4.4-integration-e2e fc2ee75 link /test v4.4-integration-e2e
ci/prow/v4.2-integration-e2e fc2ee75 link /test v4.2-integration-e2e
ci/prow/v4.3-integration-e2e fc2ee75 link /test v4.3-integration-e2e
ci/prow/v4.5-integration-e2e fc2ee75 link /test v4.5-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.

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. 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.

6 participants