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

Add script to generate plugin overview page #260

Merged
merged 5 commits into from Jul 25, 2019

Conversation

@corneliusweig
Copy link
Contributor

commented Jul 16, 2019

Close #258

@codecov-io

This comment has been minimized.

Copy link

commented Jul 16, 2019

Codecov Report

Merging #260 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #260   +/-   ##
=======================================
  Coverage   55.66%   55.66%           
=======================================
  Files          19       19           
  Lines         918      918           
=======================================
  Hits          511      511           
  Misses        355      355           
  Partials       52       52

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8867a94...5f8e809. Read the comment docs.

}

func printTableHeader(out io.Writer) {
printRow(out, "Name", "Description", "GitHub Stars")

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 16, 2019

Member

Can I recommend actually spitting out html tables?
It's often much better to have html tables in .md files as they're more reliable and bring more freedom that probably will be necessary in the future.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 16, 2019

Author Contributor

Sure. However, this also means that we should also translate the markdown used in some of the short-descriptions into html. I think a common choice for that task is blackfriday (used by hugo for example).

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 16, 2019

Member

I think this cmd/foo tool can even have its own go.mod. :)

@ahmetb

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Do you think we can have this in cmd/[name]/ instead ?

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

Do you think we can have this in cmd/[name]/ instead ?

Yeah why not.

@ahmetb

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Let's move this PR forward by moving the file to cmd/gen-plugin-list/main.go

@corneliusweig corneliusweig force-pushed the corneliusweig:w/plugin-overview branch from d2699d9 to 22e7385 Jul 24, 2019

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Let's move this PR forward by moving the file to cmd/gen-plugin-list/main.go

Moving the file was not the problem, converting the table to html was. I didn't really find a clean solution here to prevent blackfriday from wrapping its output in <p>-tags. I ended up replacing those via a regex.

@ahmetb

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

🤔 lets keep .md, then.

Decouple plugin-overview module from main krew
This prepares to add additional dependencies to plugin-overview

@corneliusweig corneliusweig force-pushed the corneliusweig:w/plugin-overview branch from 22e7385 to a6fecb6 Jul 25, 2019

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

Ok, I reverted to the pure markdown version again.

sigs.k8s.io/krew v0.2.1
)

replace sigs.k8s.io/krew => ../..

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 25, 2019

Member

I am suspecting this would cause issues if I go to a computer and cleanly do sigs.k8s.io/krew/cmd/plugin-overview.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 25, 2019

Author Contributor

Yes, probably. The problem is that I want to use the homepage field, which was added after the last krew release.
I pinned the import to the current master HEAD instead, to solve the issue you raised.

@@ -0,0 +1,12 @@
module sigs.k8s.io/krew/cmd/plugin-overview

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 25, 2019

Member

let's call this gen-plugin-overviews or generate-.
otherwise this tool's name is unclear about what it does.

)

func main() {
pluginsDir := pflag.String("plugins-dir", "", "The directory containing the plugin manifests")

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 25, 2019

Member

do you get much out of pflag vs stdlib pkg/flags here?

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 25, 2019

Author Contributor

No. This is a leftover from when this tool was not its own module.
Only the usage output looks a bit weird when using vanilla flags, though.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 25, 2019

Member

I wouldn't worry about usage as it's not a primary tool.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 25, 2019

Member

I see your point now. glog hijacks the flags smh.

Review comments
- rename to `generate-plugin-overview`
- pin krew import to specific version
- use vanilla flag instead of pflag
require (
github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b
github.com/pkg/errors v0.8.1 // indirect
sigs.k8s.io/krew v0.2.2-0.20190724210953-8867a94acd6d

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 25, 2019

Member

funny enough this doesn't build from root

$ go build ./cmd/generate-plugin-overview

go: finding sigs.k8s.io/krew/cmd/generate-plugin-overview latest
go: finding sigs.k8s.io/krew/cmd latest
can't load package: package sigs.k8s.io/krew/cmd/generate-plugin-overview: unknown import path "sigs.k8s.io/krew/cmd/generate-plugin-overview": cannot find module providing package sigs.k8s.io/krew/cmd/generate-plugin-overview

but if you ./cmd/generate-plugin-overview then go build . it works haha.

"regexp"
"strings"

"github.com/golang/glog"

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 25, 2019

Member

maybe don't import glog (and just use log.Printf+os.Exit) if it helps you avoid glog hijacking flags

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 25, 2019

Author Contributor

I tried, but it didn't really work. It's fine either way. As you said, the tool is just a little helper.

@ahmetb

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

I just tried and I like it!

Do you envision we would require the output file on krew-index repository to be up to date (as a travis check) or should we update it ourselves periodically? Earlier option sounds like it's a burden during onboarding new plugins.

Review comments
- Change header
- Add footer
- switch from `glog` to vanilla `log`
- change label for stars
@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

Do you envision we would require the output file on krew-index repository to be up to date

I wouldn't be so strict about the overview. Initially, manual sounds right, as there is

a) not so many new plugins
b) we may not keep the page

In the long run, my favorite solution would be to let a hook add additional commits, when the overview has changed. Or at least, a PR for the change is created. But that sounds like more work than we are saving for a long time.

@ahmetb

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Thanks for developing this.
/lgtm
/approve

I'm thinking we host this on krew main repo docs/plugins.md and link from the main README.md, but we can also go with krew-index (just that it ships to everyone’s machine is my only concern, but this is a small file anyway).

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 25, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Jul 25, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • OWNERS [ahmetb,corneliusweig]

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 merged commit 38672e5 into kubernetes-sigs:master Jul 25, 2019

2 of 3 checks passed

tide Not mergeable. Needs lgtm label.
Details
cla/linuxfoundation corneliusweig authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.