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

Adding more small fixes to the walkthrough & install docs#1169

Merged
duglin merged 8 commits into
kubernetes-retired:masterfrom
arschles:walkthrough-note-follow-up
Aug 30, 2017
Merged

Adding more small fixes to the walkthrough & install docs#1169
duglin merged 8 commits into
kubernetes-retired:masterfrom
arschles:walkthrough-note-follow-up

Conversation

@arschles
Copy link
Copy Markdown
Contributor

@arschles arschles commented Aug 29, 2017

This is a follow-up to #1163.

@arschles arschles requested review from MHBauer, pmorie and vaikas August 29, 2017 22:34
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 29, 2017
Comment thread docs/walkthrough-1.6.md Outdated
Notice that a new `Secret` named `ups-instance-credential` has been created.

# Step 6 - Unbinding from the ServiceInstance
# Step 6 - Deleting the `ServiceInstanceCredentials`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit, this is singular instead of plural

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch, thanks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@pmorie pmorie added the LGTM1 label Aug 30, 2017
@pmorie
Copy link
Copy Markdown
Contributor

pmorie commented Aug 30, 2017

One nit and LGTM

@arschles
Copy link
Copy Markdown
Contributor Author

Thanks for the review @pmorie

Comment thread docs/walkthrough-1.6.md Outdated
[installation instructions for 1.6](./install-1.6.md).__

# Step 1 - Installing the UPS ServiceBroker
# Step 1 - Installing the UPS Broker Server
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmmm, isn't "Service Broker" more appropriate?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

or even "Service Broker Server" if you're trying to be less OSB-API-y

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can do "Service Broker Server". I need to clearly differentiate it from the ServiceBroker resource.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread docs/walkthrough-1.6.md
# Step 6 - Deleting the `ServiceInstanceCredential`

Now, let's unbind from the instance. To do this, we simply *delete* the
Now, let's unbind from the provisioned instance. To do this, we simply *delete* the
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seems inconsistent to use 'delete' in the title but 'unbind' in the text. I'm ok with either one, but we should be consistent

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@duglin this document tries to equate Kubernetes actions (delete in this case) with the equivalent OSB concept (unbind in this case), so I kept with that trend.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as a newbie I would be confused by the switch in verbs, perhaps do something like:
Now, let's unbind from (delete) the provisioned instance

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no that's not right either....

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

skip my comment - we can tweak it in a follow-on PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@duglin if you'd like to switch the verbs around, I'm happy to do that. It'll require a change in the rest of the doc too, so how about we do that in a follow-up?

@duglin
Copy link
Copy Markdown
Contributor

duglin commented Aug 30, 2017

LGTM

@duglin duglin added the LGTM2 label Aug 30, 2017
@duglin
Copy link
Copy Markdown
Contributor

duglin commented Aug 30, 2017

waiting on CI....

@arschles
Copy link
Copy Markdown
Contributor Author

thanks @duglin

@duglin duglin merged commit 08043bd into kubernetes-retired:master Aug 30, 2017
@arschles arschles deleted the walkthrough-note-follow-up branch August 30, 2017 19:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2 size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants