Skip to content

Fix the wrong information in grpc example#1380

Merged
knative-prow-robot merged 1 commit into
knative:masterfrom
houshengbo:fix-grpc-example-error
Jun 1, 2019
Merged

Fix the wrong information in grpc example#1380
knative-prow-robot merged 1 commit into
knative:masterfrom
houshengbo:fix-grpc-example-error

Conversation

@houshengbo
Copy link
Copy Markdown

@houshengbo houshengbo commented May 29, 2019 •
edited
Loading

Fixes #1381

Proposed Changes

  • Based on the syntax in sample.yaml, it should be version 0.6 or later.
  • The command "docker push" uses a wrong tag name for the image. This PR corrects it.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 29, 2019
@knative-prow-robot knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 29, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented May 29, 2019

Hi @houshengbo. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 29, 2019
@houshengbo
Copy link
Copy Markdown
Author

houshengbo commented May 29, 2019

/assign @RichieEscarez

sebgoa
sebgoa suggested changes May 30, 2019
View reviewed changes
Comment thread docs/serving/samples/grpc-ping-go/README.md Outdated
## Prerequisites

- [Install the Knative Serving version 0.4 or later](../../../install/README.md).
- [Install the Knative Serving version 0.6 or later](../../../install/README.md).
Copy link
Copy Markdown
Contributor

@sebgoa sebgoa May 30, 2019

Choose a reason for hiding this comment

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

we could remove the version entirely that way it won't get out of date and always reference the latest install doc.

Copy link
Copy Markdown
Author

@houshengbo houshengbo May 30, 2019

Choose a reason for hiding this comment

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

👌

Copy link
Copy Markdown
Contributor

@RichieEscarez RichieEscarez May 30, 2019

Choose a reason for hiding this comment

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

my only concern here is if someone with an existing older version of Serving (pre 0.6) tries to run this

what if we say the "latest version?

Suggested change
- [Install the Knative Serving version 0.6 or later](../../../install/README.md).
- [Install the latest version of Knative Serving](../../../install/README.md).

@sebgoa
Copy link
Copy Markdown
Contributor

sebgoa commented May 30, 2019

/ok-to-test

Aside from my comment about removing the version entirely, this is

/lgtm

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. lgtm Indicates that a PR is ready to be merged. labels May 30, 2019
@sebgoa
Copy link
Copy Markdown
Contributor

sebgoa commented May 30, 2019

thanks, I think it is better like this.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 30, 2019
RichieEscarez
RichieEscarez suggested changes May 30, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@RichieEscarez RichieEscarez left a comment

Choose a reason for hiding this comment

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

added a response and requested a change regarding the required Serving version

@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented May 31, 2019

New changes are detected. LGTM label has been removed.

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 31, 2019
@houshengbo
Copy link
Copy Markdown
Author

houshengbo commented May 31, 2019

@RichieEscarez changed into latest version.

@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented May 31, 2019

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: houshengbo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@RichieEscarez RichieEscarez added the lgtm Indicates that a PR is ready to be merged. label May 31, 2019
@RichieEscarez
Copy link
Copy Markdown
Contributor

RichieEscarez commented May 31, 2019

/retest

@knative-prow-robot knative-prow-robot merged commit 3a62896 into knative:master Jun 1, 2019
RichieEscarez pushed a commit to RichieEscarez/docs that referenced this pull request Jun 6, 2019
@RichieEscarez
Fix the wrong information in grpc example (knative#1380)
e4241ad
@RichieEscarez RichieEscarez mentioned this pull request Jun 6, 2019
Merged
knative-prow-robot pushed a commit that referenced this pull request Jun 7, 2019
@RichieEscarez @knative-prow-robot
Cherry pick changes from master into 0.6 (#1423)
ddf26f8 
* pr#1408

* pr#1398

* service/samples: add .dockerignore to samples with local build disruption risk (#1392)

* service/samples: add .dockerignore to samples with risk of local build interference

* serving/samples: .dockerignore readme text update

* Update docs/serving/samples/hello-world/helloworld-csharp/README.md

Co-Authored-By: RichieEscarez <rescarez@google.com>

* Update docs/serving/samples/hello-world/helloworld-nodejs/README.md

Co-Authored-By: RichieEscarez <rescarez@google.com>

* Update docs/serving/samples/hello-world/helloworld-python/README.md

Co-Authored-By: RichieEscarez <rescarez@google.com>

* Update docs/serving/samples/hello-world/helloworld-php/README.md

Co-Authored-By: Evan Anderson <evan.k.anderson@gmail.com>

* Fix the wrong information in grpc example (#1380)

* pr#1363

* Fix samples path (#1357)

* Copy the updates from knative/eventing#1241. (#1355)

* add reference to custom DNS config (#1327)

* add reference to custom DNS config

* Update docs/install/getting-started-knative-app.md

Co-Authored-By: Sam O'Dell <31352624+samodell@users.noreply.github.com>

* fix relative link

* Fix PR#1202

* Document serving tag resolution (#1260)

Our controller does resolution of tags to digests, which has been a
source of confusion. This documents the fact that we do it, why we do
it, and how to configure the controller to work around common issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gyliu513 gyliu513 Awaiting requested review from gyliu513

2 more reviewers

@sebgoa sebgoa sebgoa requested changes

@RichieEscarez RichieEscarez RichieEscarez requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sebgoa sebgoa

@RichieEscarez RichieEscarez

Labels

cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

grpc example has an incorrect version for knative and incorrect tag to push

5 participants

@houshengbo @knative-prow-robot @sebgoa @RichieEscarez @googlebot