Skip to content

Conversation

@rh-max
Copy link
Contributor

@rh-max rh-max commented May 20, 2021

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 20, 2021
@netlify
Copy link

netlify bot commented May 20, 2021

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: 9fab681

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/60be3f0106094000084b15ae

😎 Browse the preview: https://deploy-preview-32671--osdocs.netlify.app

@rh-max
Copy link
Contributor Author

rh-max commented May 20, 2021

@dsimansk @abrennan89 Could you PTAL? Note that based on David's bug report, I have not included the command that modifies an offline service.
@dsimansk Is this a technology preview?
@abrennan89 The current placement is questionable, but I couldn't find a better place. Should I leave it here, subject to the upcoming restructuring?

https://deploy-preview-32671--osdocs.netlify.app/openshift-enterprise/latest/serverless/knative_serving/serverless-applications.html#using-kn-cli-in-offline-mode

@dsimansk
Copy link

@dsimansk Is this a technology preview?

We call it "experimental" in the upstream to have a bit of soak time and potential window to take compatibility-breaking changes. IMO it sound all right to have a note that it's technology preview feature in our docs. @rhuss any objections to call offline mode TP?

@rh-max
Copy link
Contributor Author

rh-max commented May 20, 2021

@mvinkler Could you please review these docs?

@abrennan89
Copy link
Contributor

@mvinkler Could you please review these docs?

@rh-max he's on sick leave I think so someone else may need to take a look for QE.

I think a better place for this would be under / after the serverless applications topic here: https://deploy-preview-32671--osdocs.netlify.app/openshift-enterprise/latest/serverless/knative_serving/serverless-applications.html

Only this module https://deploy-preview-32671--osdocs.netlify.app/openshift-enterprise/latest/serverless/cli_reference/kn-offline-services.html#creating-an-offline-service_kn-offline-services really belongs in the CLI guide I think, and it can live in the kn service page https://deploy-preview-32671--osdocs.netlify.app/openshift-enterprise/latest/serverless/cli_reference/kn-services-ref.html since it's a kn service command.

This one isn't a blocker for 1.15.0 FYI, so I'd make the release notes higher priority probably and come back to it if you're pushed for time.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2021
@rh-max rh-max force-pushed the srvls-offline-services branch from d2ba6f7 to 99b26a8 Compare May 26, 2021 08:00
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2021
@rh-max
Copy link
Contributor Author

rh-max commented May 26, 2021

@mvinkler Could you please review these docs?

@rh-max he's on sick leave I think so someone else may need to take a look for QE.

I think a better place for this would be under / after the serverless applications topic here: https://deploy-preview-32671--osdocs.netlify.app/openshift-enterprise/latest/serverless/knative_serving/serverless-applications.html

Only this module https://deploy-preview-32671--osdocs.netlify.app/openshift-enterprise/latest/serverless/cli_reference/kn-offline-services.html#creating-an-offline-service_kn-offline-services really belongs in the CLI guide I think, and it can live in the kn service page https://deploy-preview-32671--osdocs.netlify.app/openshift-enterprise/latest/serverless/cli_reference/kn-services-ref.html since it's a kn service command.

This one isn't a blocker for 1.15.0 FYI, so I'd make the release notes higher priority probably and come back to it if you're pushed for time.

@abrennan89 Thanks. I think [1] is a good location.

However, I think it's better to not split modules/kn-service-offline-about.adoc and modules/kn-service-offline-create.adoc. So I have put both into [1]. The reasons being:

  1. Splitting the concept and the procedure, especially to reside in different sections inside serverless/ reduces usability of the docs.
  2. Everything that's currently in [1] is a procedure. So I'd rather not put a bare concept there. A procedure prepended by a concept, however, is fine.
  3. modules/kn-service-offline-create.adoc isn't so much a reference as it is a user story. So the initial placement in serverless/cli-reference/ was really just a temporary parking. It should be in a more user story-oriented part of the docs, such as [1].

[1] https://deploy-preview-32671--osdocs.netlify.app/openshift-enterprise/latest/serverless/knative_serving/serverless-applications.html

@rh-max
Copy link
Contributor Author

rh-max commented May 26, 2021

Copy link
Contributor

@jrangelramos jrangelramos left a comment

Choose a reason for hiding this comment

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

qe review. looks good to me with minor suggestions.

In time, as a suggestion I think it would be nice to show (as a step 5.) the actual service creation in a cluster using the descriptor created. but I leave it up to you, wdyt?

$ kn service create -f test/ksvc/event-display.yaml 
Creating service 'event-display' in namespace 'test':

  0.058s The Route is still working to reflect the latest desired specification.
  0.098s ...
  0.168s Configuration "event-display" is waiting for a Revision to become ready.
 23.377s ...
 23.419s Ingress has not yet been reconciled.
 23.534s Waiting for load balancer to be ready
 23.723s Ready to serve.

Service 'event-display' created to latest revision 'event-display-00001' is available at URL:

@rh-max
Copy link
Contributor Author

rh-max commented Jun 1, 2021

qe review. looks good to me with minor suggestions.

In time, as a suggestion I think it would be nice to show (as a step 5.) the actual service creation in a cluster using the descriptor created. but I leave it up to you, wdyt?

$ kn service create -f test/ksvc/event-display.yaml 
Creating service 'event-display' in namespace 'test':

  0.058s The Route is still working to reflect the latest desired specification.
  0.098s ...
  0.168s Configuration "event-display" is waiting for a Revision to become ready.
 23.377s ...
 23.419s Ingress has not yet been reconciled.
 23.534s Waiting for load balancer to be ready
 23.723s Ready to serve.

Service 'event-display' created to latest revision 'event-display-00001' is available at URL:

Good idea, I've included this as the last step.

@rh-max
Copy link
Contributor Author

rh-max commented Jun 1, 2021

@dsimansk @jrangelramos I've implemented your feedback, which resulted in some more rewrites. I'd appreciate you having a look at the changes. No need for actual QE testing, @jrangelramos, I have not changed the commands.
Diff: cfe0ac7
Preview: https://deploy-preview-32671--osdocs.netlify.app/openshift-enterprise/latest/serverless/knative_serving/serverless-applications.html#using-kn-cli-in-offline-mode

@jrangelramos
Copy link
Contributor

QE Approved

Copy link

@dsimansk dsimansk left a comment

Choose a reason for hiding this comment

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

Thanks Max looks good to me. I like the updated introduction listing with the use cases. 👍

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 1, 2021
@rh-max
Copy link
Contributor Author

rh-max commented Jun 1, 2021

Thanks @dsimansk and @jrangelramos for your work on this!

@lbarbeevargas lbarbeevargas added the serverless Label for all Serverless PRs label Jun 1, 2021
@abrennan89
Copy link
Contributor

@rh-max please ensure that you're adding labels if you can and following the guidelines wrt titles etc for PRs. There was an email forwarded to us about this from Vikram / Cheryl.

Copy link
Contributor

@lbarbeevargas lbarbeevargas left a comment

Choose a reason for hiding this comment

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

Great job on this new section for offline mode! I added some feedback and considerations per the OCP guidelines and ISG. Please let me know if you have any questions about my comments.

@lbarbeevargas lbarbeevargas added the peer-review-done Signifies that the peer review team has reviewed this PR label Jun 1, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jun 2, 2021

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2021
@rh-max
Copy link
Contributor Author

rh-max commented Jun 2, 2021

@lbarbeevargas Thanks for the review 👍 , all suggestions implemented. Provided you, or anyone else, has no more feedback, let's consider this ready.
At the moment, this should NOT be merged. I'll let you know.

Copy link
Contributor

@abrennan89 abrennan89 left a comment

Choose a reason for hiding this comment

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

Some final updates, otherwise lgtm

@rh-max rh-max changed the title Add docs on using offline services with the kn CLI SRVCLI-259: Add docs on using offline services with the kn CLI Jun 7, 2021
Copy link
Contributor

@abrennan89 abrennan89 left a comment

Choose a reason for hiding this comment

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

lgtm!

Add Technology Preview note for using offline services

Improve feature name in TP note

Move Using offline services to serverless/knative_serving

Fix section nesting level

Numerous improvements

Add missing output line

Numerous stylistic improvements

s/the offline mode/offline mode/

Lowercase "Continuous Integration"

Co-authored-by: Ashleigh Brennan <abrennan@redhat.com>

Minor markup and wording fixes
@rh-max rh-max force-pushed the srvls-offline-services branch from 4233c4c to 9fab681 Compare June 7, 2021 15:45
@abrennan89 abrennan89 merged commit 8bb983a into openshift:master Jun 7, 2021
@abrennan89
Copy link
Contributor

/cherrypick enterprise-4.6

@abrennan89
Copy link
Contributor

/cherrypick enterprise-4.7

@abrennan89
Copy link
Contributor

/cherrypick enterprise-4.8

@openshift-cherrypick-robot

@abrennan89: new pull request created: #33157

Details

In response to this:

/cherrypick enterprise-4.6

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-cherrypick-robot

@abrennan89: new pull request created: #33158

Details

In response to this:

/cherrypick enterprise-4.7

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-cherrypick-robot

@abrennan89: new pull request created: #33159

Details

In response to this:

/cherrypick enterprise-4.8

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.6 branch/enterprise-4.7 branch/enterprise-4.8 peer-review-done Signifies that the peer review team has reviewed this PR serverless Label for all Serverless PRs size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants