Dynamic plugins list in the website #658

ahmetb · 2020-10-09T23:27:26Z

This relies on Netlify's serverless functions capability to render
plugin count (in the homepage) and plugin list (/docs/plugins) in
the website!

SIG Docs TL has cleared the usage of Functions feature here:
https://groups.google.com/g/kubernetes-sig-docs/c/cXqQm60tGs8

The serverless function is called "api" and it's accessible on path
/.netlify/functions/api/* and has two subpaths that return JSON responses:

*/pluginCount
*/plugins

This serverless function is built by placing a binary in /site/functions
named "api" and the build instruction is in 'netlify.toml' file.

Since we get plugin list by calling GitHub API, by default we run without
an authentication token. However after this is merged, I will go and add a
permissionless $GITHUB_ACCESS_TOKEN that I have created from my account.

Unauthenticated GitHub calls are limited to 60 per IP, but since I'll add a
personal access token, that should not be an issue. When debugging locally,
60 is also fine. (deploy previews for PRs from non-maintainers will not have
an access token available, so they will make unauthenticated calls to GH API.)

The responses from the dynamic functions are actually cached in Netlify's
CDN and that helps with the rate-limiting as well. I have currently set the
CDN cache duration to 1 hour for each response.

Please take a look at the previews and let me know if there is anything that
doesn't look good.

I might delete generate-plugin-overview tool and update the current
plugins.md in a followup PR.

/assign @chriskim06
/assign @corneliusweig

ahmetb · 2020-10-09T23:40:30Z

/hold
Not entirely ready for review, requires some setup. I'll ping again later.

ahmetb · 2020-10-09T23:44:31Z

/hold cancel

Ready to preview at:

Feel free to add commits to my branch.

This relies on Netlify's serverless functions capability to render plugin count (in the homepage) and plugin list (/docs/plugins) in the website! The serverless function is called "api" and it's accessible on path /.netlify/functions/api/* and has two subpaths that return JSON responses: 1. */pluginCount 2. */plugins This serverless function is built by placing a binary in /site/functions named "api" and the build instruction is in 'netlify.toml' file. Since we get plugin list by calling GitHub API, by default we run without an authentication token. However after this is merged, I will go and add a permissionless $GITHUB_ACCESS_TOKEN that I have created from my account. Unauthenticated GitHub calls are limited to 60 per IP, but since I'll add a personal access token, that should not be an issue. When debugging locally, 60 is also fine. (deploy previews for PRs from non-maintainers will not have an access token available, so they will make unauthenticated calls to GH API.) The responses from the dynamic functions are actually cached in Netlify's CDN and that helps with the rate-limiting as well. I have currently set the CDN cache duration to 1 hour for each response. Please take a look at the previews and let me know if there is anything that doesn't look good. I might delete `generate-plugin-overview` tool and update the current plugins.md in a followup PR. Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>

corneliusweig

This is really nice work! A few nits, but otherwise this is perfect stuff.

Very cool that you found out about netlify functions. This will save us so much pointless work.

I agree that generate-plugin-overview should be deleted asap as soon as this is merged.

corneliusweig · 2020-10-10T19:11:32Z

site/content/_index.md

@@ -11,7 +11,8 @@ Krew helps you:
 - install them on your machine,
 - and keep the installed plugins up-to-date.

-There are [over 90 kubectl plugins][list] currently distributed on Krew.
+There are [over <span id="krew-plugin-count">...</span> kubectl plugins][list]


Suggested change

There are [over <span id="krew-plugin-count">...</span> kubectl plugins][list]

There are [<span id="krew-plugin-count">...</span> kubectl plugins][list]

IIUC, the number is exact?

corneliusweig · 2020-10-10T19:15:17Z

site/layouts/partials/footer.html

+        fetch('/.netlify/functions/api/pluginCount')
+            .then(response => response.json())
+            .then(response => {
+                console.log(`${response.data.count} plugins found`);


I'd omit console.log for the good case. We don't want to spam the console.

corneliusweig · 2020-10-10T19:26:51Z

site/layouts/partials/footer.html

+            .then(response => response.json())
+            .then(response => {
+                console.log(`${response.data.count} plugins found`);
+                pluginCountElem.innerText=response.data?.count || 0;


Suggested change

pluginCountElem.innerText=response.data?.count || 0;

pluginCountElem.innerText=response.data?.count || '?';

This will make it clearer that there was a failure.

Might do <error> :D

corneliusweig · 2020-10-10T19:28:05Z

site/layouts/partials/footer.html

@@ -44,5 +44,77 @@
    </script>
    {{ end }}

+    <script>
+    document.addEventListener('DOMContentLoaded', (event) => {
+        var pluginCountElem = document.getElementById("krew-plugin-count");


I'd prefer to use querySelectorAll instead, and replace all occurrences on the page. I can easily imagine that we will show the plugin count in two different places and then we will need that.

corneliusweig · 2020-10-10T19:29:18Z

site/layouts/partials/footer.html

+                .then(response => response.json())
+                .then(response => {
+                    var count = response.data?.plugins?.length || 0;
+                    console.log(`${count} plugins loaded`);


nit: no logging?

corneliusweig · 2020-10-10T20:15:50Z

site/functions/server/main.go

+			for {
+				select {
+				case <-ctx.Done():
+					return
+				case url, ok := <-queue:
+					if !ok {
+						return
+					}
+					p, err := readPlugin(url)
+					if err != nil {
+						retErr = err
+						cancel()
+						return
+					}
+					mu.Lock()
+					out = append(out, p)
+					mu.Unlock()
+				}
+			}


When using errgroup, this could be simplified to

for url := range queue { p, err := readPlugin(url) if err != nil { return err } mu.Lock() out = append(out, p) mu.Unlock() }

might use errgroup, but I don't see a way to limit parallelism with errgroup.
feel free to add a patch.

corneliusweig · 2020-10-10T20:16:35Z

site/functions/server/main.go

+func readPlugin(url string) (*krew.Plugin, error) {
+	resp, err := http.Get(url)
+	if err != nil {
+		return nil, errors.Wrapf(err,"failed to get %s", url)


nit: run gofmt :)

corneliusweig · 2020-10-10T20:19:31Z

site/functions/server/main.go

+	if resp.Body != nil {
+		defer resp.Body.Close()
+	}


nit:

Suggested change

if resp.Body != nil {

defer resp.Body.Close()

}

defer resp.Body.Close()

From the spec:

// The http Client and Transport guarantee that Body is always
// non-nil, even on responses without a body or responses with
// a zero-length body. It is the caller's responsibility to
// close Body.

corneliusweig · 2020-10-10T20:20:13Z

site/functions/server/main.go

+	if resp.Body != nil {
+		defer resp.Body.Close()
+	}
+	var v krew.Plugin


nit: move v right before yaml.Unmarshal

corneliusweig · 2020-10-10T20:23:01Z

site/functions/server/main.go

+	// To debug locally, you can run this server with -port=:8080 and run "hugo serve" and uncomment this:
+	mux.Handle("/", httputil.NewSingleHostReverseProxy(&url.URL{Scheme: "http", Host: "localhost:1313"}))


The comment reads like this should be commented out?

Anyway, I think it'd be much nicer like that, then no commenting is necessary:

if *port > -1 { ... }

It can be commented out but I left it on just in case. You're right.

ahmetb · 2020-10-12T23:14:42Z

@corneliusweig I think it would be better if you do a pass with your commits on the frontend pieces. My frontend skills are weak at best. :)

corneliusweig · 2020-10-16T10:16:36Z

@ahmetb I added some commits but had to create a PR to your repo: ahmetb#2
Please have a look

ahmetb · 2020-10-16T17:42:46Z

🤔 I see "Allow edits and access to secrets by maintainers" so you should be able to push to my branch. Have you tried over ssh (git://) ?

site/functions/server/main.go

corneliusweig · 2020-10-17T18:49:06Z

So I tried again various approaches---nothing will update this PR. However, now there's also a new branch in krew, called netlify-dynamic-content :(.

So I think it's easiest if you merge my PR into your PR 🙄

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

ahmetb · 2020-10-18T04:25:51Z

site/functions/go.mod

+	golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be
+	golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4
+	gopkg.in/yaml.v2 v2.2.8
+	sigs.k8s.io/krew v0.4.0


this is a bit worrisome to pin the site to a particular version of master. (should be ok as long as we keep the types/consts backwards compatible)

this replace statement works fine locally, but once I uncommented the replace statement and deploy, it fails to build on Netlify sadly. not sure why.

Yes, it will be annoying if we forget about it. I don't see a better alternative though.

ahmetb · 2020-10-18T04:28:19Z

site/functions/server/main.go

+	knownHomePages := map[string]string{
+		`https://krew.sigs.k8s.io/`:                                  "kubernetes-sigs/krew",
+		`https://sigs.k8s.io/krew`:                                   "kubernetes-sigs/krew",
+		`https://kubernetes.github.io/ingress-nginx/kubectl-plugin/`: "kubernetes/ingress-nginx",
+		`https://kudo.dev/`:                                          "kudobuilder/kudo",
+		`https://kubevirt.io`:                                        "kubevirt/kubectl-virt-plugin",
+		`https://popeyecli.io`:                                       "derailed/popeye",
+		`https://soluble-ai.github.io/kubetap/`:                      "soluble-ai/kubetap",
+	}


@corneliusweig I wonder if we should move this to somewhere like a var declaration at the top. I suspect it'll get updated frequently. :D

I wonder if we should even go one step further and extract this configuration into non-code. In krew-index-tracker, I also need this list and it is error-prone to redeploy every time this changes.

Maybe we can keep this map in a very dumb format in a text file inside krew-index or krew. Something like

https://krew.sigs.k8s.io/ kubernetes-sigs/krew https://sigs.k8s.io/krew kubernetes-sigs/krew ...

I'm -1 for a new file/format.
I think we can hardcode these still in a centralized source code file.
We can do a variable on top of main.go for now.

ahmetb · 2020-10-18T04:28:43Z

site/layouts/partials/footer.html

@@ -44,5 +44,58 @@
    </script>
    {{ end }}

+    <script>


@corneliusweig should we move these to a .js file at this point?

Yes, that makes sense to me. We don't have to do it in this PR though. Maybe open a new issue? [[main reason: collaboration on the shared PR seems to be not straightforward]]

ahmetb · 2020-10-18T04:29:14Z

site/layouts/partials/footer.html

+    </script>
+
+    <script type="module">
+    import {html, render} from 'https://unpkg.com/lit-html?module';


I must admit I don't fully understand how imports work in the browser so @corneliusweig you’ll probably be on point for maintaining these :D I barely can get the vanilla js working.

Yeah, I'm not too worried about it (support stats).

corneliusweig · 2020-10-18T14:50:09Z

site/functions/server/main.go

+}
+
+func main() {
+	port := flag.Int("port", -1, `To debug locally set --port=8080 and run "hugo serve"`)


Running the server locally with hugo did not work for me, I always got not found, because the site was trying to query the server on port 1313 (hugo) instead of 8080. Can you write down the instructions in a step-by-step manner?

pushed a ./site/functions/README.md . PTAL

chriskim06 · 2020-10-18T15:08:08Z

site/layouts/partials/footer.html

+        try {
+            const resp = await fetch('/.netlify/functions/api/pluginCount');
+            const json = await resp.json();
+            count = json.data?.count ?? '<error>';


It seems like optional chaining is relatively new (or at least those version numbers seem a little high). Do either of you think this should be changed to the old way or is it ok?

The question immediately becomes: how far back do we want to be backwards compatible.

This case is trivial, but import and async/await is already more involved and the old style quickly becomes kind of ugly. We could warn page visitors by including a <script nomodule ..> script to inform the user that the page is not working as intended and to switch to a newer browser.

I think these are mostly fine. I tend to assume developers use latest browser versions most of the time. In that regard they're more savvy than average computer user.

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

ahmetb · 2020-10-21T20:52:37Z

added two more commits:

543f132 move from /docs/plugins to /plugins → this way plugin list isn't part of docs
fb0b188 add /plugins to navbar → show a "Plugins" item on the top navigation bar.

corneliusweig · 2020-10-24T21:54:02Z

Oh no! I'm so slow. Apologies 🙇‍♂️
/lgtm
/approve

k8s-ci-robot · 2020-10-24T21:54:14Z

[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 assigned chriskim06 and corneliusweig Oct 9, 2020

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 9, 2020

k8s-ci-robot requested review from corneliusweig and soltysh October 9, 2020 23:27

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 9, 2020

ahmetb force-pushed the netlify-dynamic-content branch 5 times, most recently from 8f53f0d to bbeed9c Compare October 9, 2020 23:40

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2020

ahmetb force-pushed the netlify-dynamic-content branch from bbeed9c to a0517a7 Compare October 9, 2020 23:42

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2020

ahmetb force-pushed the netlify-dynamic-content branch from a0517a7 to 66f683b Compare October 9, 2020 23:46

ahmetb mentioned this pull request Oct 10, 2020

site: Show dynamic plugin list on a website page #519

Closed

corneliusweig approved these changes Oct 10, 2020

View reviewed changes

chriskim06 reviewed Oct 16, 2020

View reviewed changes

site/functions/server/main.go Show resolved Hide resolved

corneliusweig added 4 commits October 17, 2020 20:23

Apply review comments

59a65d9

Use lit-html and async/await style for better readability

bf10f65

Simplify work sharing with errgroup

871f617

Satisfy impi constraints

6796c89

fix import

82c1f86

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

ahmetb commented Oct 18, 2020

View reviewed changes

corneliusweig reviewed Oct 18, 2020

View reviewed changes

chriskim06 reviewed Oct 18, 2020

View reviewed changes

ahmetb added 3 commits October 19, 2020 15:12

add setup/debugging documentation

9100f2e

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

move from /docs/plugins to /plugins

543f132

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

add /plugins to navbar

fb0b188

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

ahmetb mentioned this pull request Oct 21, 2020

Redirect plugin list to the site kubernetes-sigs/krew-index#882

Merged

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2020

k8s-ci-robot merged commit 53aa899 into kubernetes-sigs:master Oct 24, 2020

shuuji3 mentioned this pull request Nov 19, 2020

Update URL of krew plugin list kubernetes/website#25105

Merged

	There are [over <span id="krew-plugin-count">...</span> kubectl plugins][list]
	There are [<span id="krew-plugin-count">...</span> kubectl plugins][list]

	pluginCountElem.innerText=response.data?.count \|\| 0;
	pluginCountElem.innerText=response.data?.count \|\| '?';

		// To debug locally, you can run this server with -port=:8080 and run "hugo serve" and uncomment this:
		mux.Handle("/", httputil.NewSingleHostReverseProxy(&url.URL{Scheme: "http", Host: "localhost:1313"}))

Dynamic plugins list in the website #658

Dynamic plugins list in the website #658

Conversation

ahmetb commented Oct 9, 2020 • edited Loading

ahmetb commented Oct 9, 2020

ahmetb commented Oct 9, 2020 • edited Loading

corneliusweig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb commented Oct 12, 2020

corneliusweig commented Oct 16, 2020

ahmetb commented Oct 16, 2020

corneliusweig commented Oct 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corneliusweig Oct 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb commented Oct 21, 2020

corneliusweig commented Oct 24, 2020

k8s-ci-robot commented Oct 24, 2020

ahmetb commented Oct 9, 2020 •

edited

Loading

ahmetb commented Oct 9, 2020 •

edited

Loading

corneliusweig Oct 18, 2020 •

edited

Loading