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.

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

site: custom index docs #618

Merged
merged 15 commits into from
Jul 8, 2020

Conversation

chriskim06
Copy link
Member

Let me know what you guys think. Also what do I need to do to mark this as draft?

Related issue: #566

/area multi-index
/area docs

@k8s-ci-robot k8s-ci-robot added area/multi-index area/docs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 1, 2020
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 1, 2020
@chriskim06 chriskim06 changed the title site: Custom index docs site: custom index docs Jul 1, 2020
@chriskim06 chriskim06 mentioned this pull request Jul 1, 2020
22 tasks
@ahmetb
Copy link
Member

ahmetb commented Jul 1, 2020

I'll make a pass by adding commits, stay tuned.

@ahmetb
Copy link
Member

ahmetb commented Jul 1, 2020

Just did a pass by:

  • merge 2 user guides into 1
  • remove some cmd outputs that weren't useful
  • rewrite some paragraphs
  • change titles to be task oriented "Using Custom Plugin Indexes" and "Hosting Custom Plugin Indexes"
  • emphasis on calling them "Custom Plugin Indexes" and not "custom indexes" which causes a new term to be coined.

@ahmetb
Copy link
Member

ahmetb commented Jul 1, 2020

Actually could not push the commit. @chriskim06 do you mind choosing "allow maintainers to push additional commits" on the right?

@ahmetb
Copy link
Member

ahmetb commented Jul 1, 2020

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@ahmetb
Copy link
Member

ahmetb commented Jul 1, 2020

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@ahmetb
Copy link
Member

ahmetb commented Jul 1, 2020

/lgtm
/approve
/hold

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 1, 2020
@ahmetb ahmetb added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2020
@ahmetb
Copy link
Member

ahmetb commented Jul 1, 2020

/lgtm cancel

we need to add draft: true to frontmatters

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2020
@chriskim06
Copy link
Member Author

Is that setting external to this PR?

@ahmetb
Copy link
Member

ahmetb commented Jul 1, 2020

No, you'll need to update the .md files you added and add the property to their header section.

Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

Reads pretty good. Just small nits from my side.

site/content/docs/user-guide/using-custom-indexes.md Outdated Show resolved Hide resolved
site/content/docs/user-guide/using-custom-indexes.md Outdated Show resolved Hide resolved
Comment on lines 46 to 54
## Duplicate plugin names

Your custom index can contain plugins that have the same name as the ones in
`krew-index`.

Users of your index will need to install your plugin using the
explicit `<INDEX>/<PLUGIN>` syntax. See the [user guide][ug].

[ug]: {{< relref "../user-guide/using-custom-indexes.md">}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This has considerable overlap with using-custom-indexes, and duplicates some information. Do we really need it?

(also the heading does not fit to the second paragraph)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm ok with whatever. I think it might be good to have it repeated here as a consideration for people writing plugins for custom indexes and to let them know that people will need to install their plugin differently.

However, some plugin authors may choose to host their own indexes that contain
their curation of kubectl plugins. These are called "custom plugin indexes".

## Adding a custom index
Copy link
Member

Choose a reason for hiding this comment

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

I think the title isn’t good.
It would be better to explain in this section “The default index” and

  • how it can be removed
  • or pointed to another repo
  • and how plugins without an explicit index name mean “default”

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like the adding/removing/listing indexes sections are important to showcase the new index command right? I agree though that a section explaining the default index would be good, I can move the advanced usage section I had and change it to reflect the points you listed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think this should work well. You could also explain how plugin names are resolved. Iow, that a default/ prefix is added implicitly, if no index name is given. Then you could also explain how plugin name clashes are handled and remove the Duplicate plugin names section in the other page.

foo https://github.com/foo/custom-index.git{{</output>}}
```

## The default index
Copy link
Member

Choose a reason for hiding this comment

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

Can we perhaps use code block notation for all “default”s in this paragraph?


## The default index

When a plugin doesn't have an explicit `INDEX_NAME` prefix it refers to a plugin
Copy link
Member

Choose a reason for hiding this comment

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

, it

with the same name across different indexes.

Krew ships with [`krew-index`][ki] as the default index, but this can be removed
by passing `default` to the remove command. Once this is removed, you can add
Copy link
Member

Choose a reason for hiding this comment

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

...Removed using the “kubectl krew index remove default” command

foo https://github.com/foo/custom-index.git{{</output>}}
```

## The default index
Copy link
Member

Choose a reason for hiding this comment

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

This section should ideally come after installing plugins from custom indexes (keep in mind most people might not care about the default index nuances)

@ahmetb
Copy link
Member

ahmetb commented Jul 8, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, chriskim06

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

@ahmetb
Copy link
Member

ahmetb commented Jul 8, 2020

/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 Jul 8, 2020
@k8s-ci-robot k8s-ci-robot merged commit e6d7fb6 into kubernetes-sigs:master Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/docs area/multi-index cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants