-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Updates to services docs #3460
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
Updates to services docs #3460
Conversation
mpetason
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small updates. It looks good. The only thing I really saw was related to something that was written a while ago, that could be a little bit more clear - the image section. Other than that Service is lowercase in some areas and uppercase in others - for Kubernetes Service and Knative Service.
evankanderson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions, but the move and rewrite overall looks good.
FWIW, it would be super-easy to review a file move (separate from content changes), but I realize that may be harder to handle on your end.
| - name: TARGET | ||
| value: "Go Sample v1" | ||
| ``` | ||
| * `apiVersion`: The current Knative version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we call out apiVersion but not kind. Both of these should be stable in all these examples at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk either, this was already there before I did the edits so I'd prefer not to block the PR on it since I think this whole bit could do with rewriting / reformatting to fit properly what we're doing now with "callouts" - can you maybe add a follow up issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't want to rewrite this, I'm happy to have this be the place where we draw the line on updates.
|
/approve |
|
/lgtm |
|
@pmbanugo: changing LGTM is restricted to collaborators In response to this:
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. |
evankanderson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
| --- | ||
|
|
||
| Knative Services are used to deploy an application. Each Knative Service is defined by a Route and a Configuration, which have the same name as the Service, contained in a YAML file. Every time the Configuration is updated, a new Revision is created. | ||
| To create an application using Knative, you must create a YAML file that defines a Knative service. This YAML file specifies metadata about the application, points to the hosted image of the app and allows the service to be configured. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to /lgtm and /approve, but I'm not sure about the "you must create a YAML file", since we also support using the kn CLI client and may have other mechanisms (web form?) in the future.
| - name: TARGET | ||
| value: "Go Sample v1" | ||
| ``` | ||
| * `apiVersion`: The current Knative version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't want to rewrite this, I'm happy to have this be the place where we draw the line on updates.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abrennan89, evankanderson 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 |
Fixes #978