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.

Sign up for GitHub

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

Jump to bottom

refactor python hw with tabs #2839

Merged
merged 9 commits into from Feb 2, 2021
Merged

refactor python hw with tabs #2839

merged 9 commits into from Feb 2, 2021

Conversation

itsmurugappan
Copy link
Contributor

@itsmurugappan itsmurugappan commented Sep 22, 2020 •
edited

Proposed Changes

  • hello world python example with tabs

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 22, 2020
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 22, 2020
@markusthoemmes
Copy link
Contributor

@itsmurugappan the files don't match up anymore, see the failing test for more info.

@itsmurugappan
Copy link
Contributor Author

@markusthoemmes have put back Readme.md to fix the tests.
@abrennan89 have made the same changes you proposed for java spring boot example. Please take a look.

@abrennan89
Copy link
Contributor

/approve

Looks good to me, but I'll let @markusthoemmes or @vagababov do a technical review and add a LGTM when they're approving 😉

Thanks @itsmurugappan !

vagababov
vagababov reviewed Nov 25, 2020
View reviewed changes
Copy link
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

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

LG in general

docs/serving/samples/hello-world/helloworld-python/index.md Outdated Show resolved Hide resolved
docs/serving/samples/hello-world/helloworld-python/index.md Outdated Show resolved Hide resolved
docs/serving/samples/hello-world/helloworld-python/index.md

## Verification

1. Run one of the followings commands to find the domain URL for your service.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to remark that xip.io address will be only if the default domain job is running alongside?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure, as we have marked this as example.

Copy link
Contributor

Choose a reason for hiding this comment

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

well still... people might expect xip.io and it won't be there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vagababov added a note for this.

@abrennan89
Copy link
Contributor

abrennan89 commented Jan 28, 2021 •
edited

Couple of comments:

  1. http://docs-2839.default.docs-on-the-rocks.io/docs/serving/samples/hello-world/helloworld-python/#removing looks like the kubectl tab has a kn command?
  2. http://docs-2839.default.docs-on-the-rocks.io/docs/serving/samples/hello-world/helloworld-python/#build step 5 in this procedure shows as step 1 again, I think it's because the note isn't indented properly?

Otherwise looks good, @vagababov did you want any changes re your comment on xip.io?

vagababov
vagababov reviewed Feb 1, 2021
View reviewed changes
docs/serving/samples/hello-world/helloworld-python/index.md

{{< tabs name="helloworld_python" default="kn" >}} {{% tab name="yaml" %}}

1. Create a new file, `service.yaml` and copy the following service
Copy link
Contributor

Choose a reason for hiding this comment

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

Not everything goes to docker, so note that "or with the URL provided by your container registry", or thereabouts.

docs/serving/samples/hello-world/helloworld-python/index.md

## Verification

1. Run one of the followings commands to find the domain URL for your service.
Copy link
Contributor

Choose a reason for hiding this comment

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

well still... people might expect xip.io and it won't be there

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrennan89, itsmurugappan

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

@itsmurugappan
Copy link
Contributor Author

Couple of comments:

  1. http://docs-2839.default.docs-on-the-rocks.io/docs/serving/samples/hello-world/helloworld-python/#removing looks like the kubectl tab has a kn command?
  2. http://docs-2839.default.docs-on-the-rocks.io/docs/serving/samples/hello-world/helloworld-python/#build step 5 in this procedure shows as step 1 again, I think it's because the note isn't indented properly?

Otherwise looks good, @vagababov did you want any changes re your comment on xip.io?

have addressed both the comments

@vagababov
Copy link
Contributor

I've added two nits, but otherwise looks ok.
I think the XIP problem is solvable by anyone who does knative setup first.

@itsmurugappan
Copy link
Contributor Author

I've added two nits, but otherwise looks ok.
I think the XIP problem is solvable by anyone who does knative setup first.

I have corrected that, also added the note for xip.

vagababov
vagababov reviewed Feb 2, 2021
View reviewed changes
Copy link
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2021
@knative-prow-robot knative-prow-robot merged commit 63b2e4a into knative:master Feb 2, 2021
@itsmurugappan itsmurugappan deleted the tabs-python branch February 2, 2021 22:34
RichieEscarez pushed a commit to RichieEscarez/docs that referenced this pull request Mar 6, 2021
@itsmurugappan @RichieEscarez
refactor python hw with tabs (knative#2839)
c8202ea 
* refactor python hw with tabs

* fix for tests failure

* fix for tests failure

* fix for tests failure

* address review comments

* review comments

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

@vagababov vagababov vagababov left review comments

@markusthoemmes markusthoemmes markusthoemmes left review comments

Assignees

@vagababov vagababov

Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@itsmurugappan @markusthoemmes @abrennan89 @knative-prow-robot @vagababov @googlebot