-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Reworked shell hello world example #2832
Reworked shell hello world example #2832
Conversation
rhuss
commented
Sep 18, 2020
- Reworked example to be a "true" shell example that is considerabledifferent than the helloworld-go example
- Added examples for kn usage
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
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.
Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)
@googlebot I fixed it |
55812a7
to
9b29998
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
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.
Nice work. I'll model the ruby
example to match this. Left a few comments and typo/fixes to be made.
Otherwise LGTM.
- A Kubernetes cluster with Knative installed and DNS configured. Follow the | ||
[installation instructions](../../../../install/README.md). | ||
- [Docker](https://www.docker.com) installed and running on your local machine, | ||
and a Docker Hub account 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.
Since Github supports image registry with GitHub Docker packages I wonder if we should also include that here since it keeps everything tidy with GH. Thoughts?
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'd stick with DockerHub for now. If we switch to GHCR we should switch everything, I think.
|
||
During the creation of your service, Knative performs the following steps: | ||
|
||
- Create a new immutable revision for this version of the app. |
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.
Wonder if this should be "Creates" since it really is meant to be: "Knative creates ..." and so on, so "Automatically scales ..."
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.
good question. I haven't added that part, but maybe the gerund form ("Creating") is a better fit ? (as in the next bullet point "programming" is used, but then we should also change to "scaling" in the third item.
@abrennan89 any recommendation ?
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.
"Creation" could be also an option
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 moved now to "Creates", "Programs the network" and "scales"
@maximilien If we are good with this PR I think it can be merged. @abrennan89 Maybe you could give this PR a quick look and give if it a lgtm + approve if ok ? |
@rhuss I think it is still missing a few updates. It still shows helloworld-go, maybe this is missing a commit? |
@rhuss is this for 0.19.0, and is there an issue tracking it that you can link? |
Generally this LGTM. I'm rebasing my change on this since you fixed the test, and working through the comments now. Do you want to rebase this, so we can try to land it tomorrow? |
Sorry for being later, but as said, I'm think we are good to merge this PR, nothing left from my side. Let me rebase it ... |
/lgtm |
It looks like there is a conflict?
|
9b29998
to
1db3147
Compare
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor, rhuss 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 |
1db3147
to
a1726ab
Compare
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
…tive#2916) If the type of the event isn't changed by the last step in the sequence the event will be resent to the Trigger that targets the sequence creating an unwanted loop. Fixes knative#2851 Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
* Reworked example to be a "true" shell example that is considerable different than the helloworld-go example * Added examples for kn usage (Fixed according to review comments)
@googlebot I signed it! |
a1726ab
to
39915a6
Compare
/lgtm |
* Change event type in Sequence with Broker example (knative#2897) (knative#2916) If the type of the event isn't changed by the last step in the sequence the event will be resent to the Trigger that targets the sequence creating an unwanted loop. Fixes knative#2851 Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Reworked shell hello world example * Reworked example to be a "true" shell example that is considerable different than the helloworld-go example * Added examples for kn usage (Fixed according to review comments) Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>