Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Change creates statements in walkthrough.md #2695

Merged
merged 1 commit into from
Aug 27, 2019

Conversation

mszostok
Copy link
Contributor

@mszostok mszostok commented Aug 26, 2019

Description

  • Change creates statements in walkthrough.md. Thanks to that command can be directly copied and executed in terminal without cloning the service-catalog source code
  • fix namespace name in cleaning up service catalog step
  • fix typo

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 26, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mszostok

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2019
Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

is okay-ish.

@@ -8,9 +8,6 @@ If you haven't, please see the [installation instructions](./install.md). Option
the Service Catalog CLI, svcat. Examples for both svcat and kubectl are provided
so that you may follow this walkthrough using svcat or using only kubectl.

All commands in this document assume that you're operating out of the root
of this repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

"... assume internet connectivity. Download the yaml ahead of time to etc etc etc."

Some people have to proxy in or out to github.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but basically, k8s is doing that on their examples:

kubectl apply -f https://k8s.io/examples/controllers/nginx-deployment.yaml

but just using that with k8s.io domain.
source: https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#creating-a-deployment

In the future, we can also host examples under https://svc-cat.io site.

thanks to that you can copy-paste command directly to terminal without service catalog sources, so it's huge pros. What's more, you will know that the newest version was used.
In case of doing that with checkout repo, someone can have an old version (didnt execute git pull for a long time)

but as you said such an approach requires the connection connectivity

So what is your idea? Leave it as it is and required that use checkout the service catalog locally as it was before?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes, the walkthrough is hosted.
https://svc-cat.io/docs/walkthrough/

Would be good to have it hosted on the domain, but not a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I will implement that in next pull-requests

@@ -23,7 +20,7 @@ We plan on using the minibroker for demo purposes. The codebase for that broker
[here](https://github.com/kubernetes-sigs/minibroker).

We're going to deploy the minibroker to our Kubernetes cluster before
proceeding, and we'll do so with the minibroker helm chart. You can find details about the chart in the minibroker README
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch, dupe word.

@@ -480,7 +477,7 @@ Delete the helm deployment and the namespace:

```console
helm delete --purge catalog
kubectl delete ns svc-cat
Copy link
Contributor

Choose a reason for hiding this comment

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

This should link back to other instructions rather than document how to clean up something else here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree 👍

Copy link
Contributor Author

@mszostok mszostok Aug 27, 2019

Choose a reason for hiding this comment

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

FYI: The best approach was to remove that thing. At the beginning of that walkthrough we have a prerequisite:
This document assumes that you've installed Service Catalog onto your cluster.

so I do not see any reason why we should have the delete action for SC at the end of the walkthrough. We can delete the minibroker because we are creating that during the walkthrough.

so the final shape:

# Step 1 - Installing the minibroker Server
# Step 2 - Viewing ClusterServiceClasses and ClusterServicePlans
# Step 4 - Creating a New ServiceInstance
# Step 5 - Requesting a ServiceBinding to use the ServiceInstance
# Step 6 - Deleting the ServiceBinding
# Step 7 - Deleting the ServiceInstance
# Step 8 - Deleting the ClusterServiceBroker
# Step 9 - Final Cleanup - delete minibroker and created namespaces

thanks to that the walkthrough is exactly about showing usage of SC

@MHBauer
Copy link
Contributor

MHBauer commented Aug 26, 2019

seems fine. forward progress.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2019
@mszostok
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 27, 2019
- use URL to GitHub raw YAML content instead of local file
- fix namespace name in cleaning up service catalog step
@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2019
@piotrmiskiewicz
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2019
@mszostok
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 27, 2019
@k8s-ci-robot k8s-ci-robot merged commit d1e4d14 into kubernetes-retired:master Aug 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants