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 page to list all KEPs #324

Merged
merged 3 commits into from
Oct 19, 2022
Merged

Conversation

palnabarun
Copy link
Member

@palnabarun palnabarun commented Jul 27, 2022

Part of kubernetes/enhancements#2095

The proposed change now uses Datatables for table search, pagination and sorting. No additional custom JS is required. Also, there is no change done to the content generator hack script.

Here is how the page looks:
image

Supersedes #222 (Thanks, @shekhar-rajak for the initial work 🎉 )

Note: A lot of future improvements can be done on this page. This is the MVP that we should get to first.

@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 Jul 27, 2022
@palnabarun
Copy link
Member Author

/assign @sftim @mrbobbytables

(since you reviewed #222)

@sftim
Copy link
Contributor

sftim commented Jul 27, 2022

Some of the approach in kubernetes/website#35228 might be useful for generating a list of KEPs.

@palnabarun
Copy link
Member Author

@sftim -- we are generating anything here. The KEPs list is pre-generated using a post submit for k/enhancements. We just consume the JSON here.

Not generating anything at build time for contributor-site reduces the complexity.

@sftim
Copy link
Contributor

sftim commented Jul 27, 2022

I think this is OK.

/hold

There might be other pieces that need to land before this goes in.

  • I'd prefer to be able to list all the KEPs on the final page, rather than only see 100 at a time maximum. We can fix that later.
  • The sig names should be, eg “SIG Node” rather than “sig-node”. We can apply some deterministic capitalization rules to make that happen (ideally, during JSON generation: make an extra field with the right capitalization).
  • For me, the links to pages like https://kep.k8s.io/1234 trigger a certificate name mismatch warning. We need a new cert with the right subjectAltName, I think.

/lgtm
(other folk: feel free to cancel that LGTM if any of those concerns justify it, or you spotted some other problem)

@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 Jul 27, 2022
layouts/shortcodes/keps-data.html Outdated Show resolved Hide resolved
linktitle: Enhancements
title: Kubernetes Enhancement Proposals (KEPs)
description: |
List of enhancements that are proposed
Copy link
Contributor

@sftim sftim Jul 27, 2022

Choose a reason for hiding this comment

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

Suggested change
List of enhancements that are proposed
List of Kubernetes enhancements.

“proposed” doesn't capture that some of these are fully implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

In essence, all enhancements are proposals in various states of implementation, but I agree that the language can create some confusion.

layouts/shortcodes/keps-data.html Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 27, 2022
@sftim
Copy link
Contributor

sftim commented Jul 27, 2022

/lgtm

Can we skip KEPS with a KEP ID < 1?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2022
@palnabarun
Copy link
Member Author

I'd prefer to be able to list all the KEPs on the final page, rather than only see 100 at a time maximum. We can fix that later.

The selector contains a "All" option now to cater to this.

The sig names should be, eg “SIG Node” rather than “sig-node”. We can apply some deterministic capitalization rules to make that happen (ideally, during JSON generation: make an extra field with the right capitalization).

I like this idea. I would prefer keeping just the SIG name though in this form "Node" since the table header is itself SIG. Let me try something here.

For me, the links to pages like https://kep.k8s.io/1234 trigger a certificate name mismatch warning. We need a new cert with the right subjectAltName, I think.

Moved to https://features.k8s.io/ to resolve this for now.

@palnabarun
Copy link
Member Author

Can we skip KEPS with a KEP ID < 1?

I am going to fix those. Those KEPs needs to have their metadata updated. There's also a KEP with number "NNNN".

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 27, 2022
@palnabarun
Copy link
Member Author

I like this idea. I would prefer keeping just the SIG name though in this form "Node" since the table header is itself SIG. Let me try something here.

Tried a middle ground solution. strings.TrimPrefix "sig-" $data.owningSig | humanize would render sig-contributor-experience to Contributor experience. Ideally I would want first letter of every word to be capitalized.

@sftim
Copy link
Contributor

sftim commented Jul 27, 2022

For commit 98ba82d, please (this is important) explain why you are setting a different build interval.

@sftim
Copy link
Contributor

sftim commented Jul 27, 2022

Try title rather than humanize @palnabarun

@palnabarun
Copy link
Member Author

Try title rather than humanize

Using either of them wouldn't have solved the problem since api-machinery would transform to Api-Machinery if only using title.

I solved the problem by using both in order.

Copy link
Member

@PushkarJ PushkarJ left a comment

Choose a reason for hiding this comment

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

This is great. Just a couple of minor suggestion (okay to skip making those changes before merging)

layouts/shortcodes/keps-data.html Outdated Show resolved Hide resolved
layouts/shortcodes/keps-data.html Outdated Show resolved Hide resolved
content/en/resources/keps/_index.md Outdated Show resolved Hide resolved
layouts/shortcodes/keps-data.html Outdated Show resolved Hide resolved
layouts/shortcodes/keps-data.html Outdated Show resolved Hide resolved
layouts/shortcodes/keps-data.html Outdated Show resolved Hide resolved
layouts/shortcodes/keps-data.html Outdated Show resolved Hide resolved
@dims
Copy link
Member

dims commented Aug 22, 2022

howdy folks! how far away are we from merging this and then iterating? 🙏🏾

@sftim
Copy link
Contributor

sftim commented Aug 22, 2022

The one thing I'd like to see added is the comment I asked for in https://github.com/kubernetes/contributor-site/pull/324/files#r945321372

Other details we could fix post-merge. I'm asking for the comment because if that post-merge tidying never happens, we actually lose some important context.

@palnabarun palnabarun force-pushed the kep-website branch 3 times, most recently from 9c7cc02 to 137b312 Compare September 14, 2022 09:51
@palnabarun
Copy link
Member Author

I have catered to all the suggestions except the one about the location of the stylesheet and scripts specific to the shortcode.

@sftim -- I agree that having the stylesheet and scripts in the body is an antipattern and shouldn't be done. On checking where the header is defined, it's in the theme layouts. So, moving the stylesheet and scripts there would mean replicating the head.html and head-css.html partial from the theme to the sites layout/partials directory. In future, this may lead to maintenance overhead. What do you suggest to mitigate this issue?

@jberkus
Copy link
Contributor

jberkus commented Sep 14, 2022

Can you remove the top-level menu item on the home page?

@sftim
Copy link
Contributor

sftim commented Sep 14, 2022

replicating the head.html and head-css.html partial from the theme to the sites layout/partials directory.

There are some ways in Hugo to implement code reuse. If we find there's a lot of maintenance overhead, we can consider that.

https://github.com/kubernetes/website/ uses the same Docsy theme (much more customized) and you might find some tips there about how to override just part of an inherited partial.

For scripts, you can often trigger these to load from the page footer.

@sftim
Copy link
Contributor

sftim commented Sep 15, 2022

A tip: edit the PR description to include a link to the preview

Signed-off-by: Nabarun Pal <pal.nabarun95@gmail.com>
Signed-off-by: Nabarun Pal <pal.nabarun95@gmail.com>
Signed-off-by: Nabarun Pal <pal.nabarun95@gmail.com>
@palnabarun
Copy link
Member Author

palnabarun commented Sep 15, 2022

@sftim @jberkus -- this should be good to merge now. Can you please take a look?

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm


{{% alert title="Note" color="info" %}}
If you notice an empty cell in the table, the KEP may not have updated information.
Please raise <a href="https://github.com/kubernetes/enhancements/issues">an issue in the kubernetes/enhancemenets</a> repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Please raise <a href="https://github.com/kubernetes/enhancements/issues">an issue in the kubernetes/enhancemenets</a> repository.
Please raise <a href="https://github.com/kubernetes/enhancements/issues">an issue in the kubernetes/enhancements</a> repository.

@@ -0,0 +1,25 @@
{{/*
This file is a 1:1 copy of the one in docsy but with adding scripts
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: which Docsy release / commit is this from?


<div>
<table id="keps-table" class="table table-hover mt-2 col-md-12 table-sm">
<thead class="thead-light">
Copy link
Contributor

Choose a reason for hiding this comment

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

Optionally: add a table caption.

*/}}

<div>
<table id="keps-table" class="table table-hover mt-2 col-md-12 table-sm">
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: If someone were to use that shortcode twice in one page, the output would have a duplicated id. I think this is low risk low impact.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2022
Copy link
Member

@mrbobbytables mrbobbytables left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrbobbytables, palnabarun

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 Oct 19, 2022
@sftim
Copy link
Contributor

sftim commented Oct 19, 2022

Let's ship it

/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 Oct 19, 2022
@k8s-ci-robot k8s-ci-robot merged commit 7921c0c into kubernetes:master Oct 19, 2022
@palnabarun palnabarun deleted the kep-website branch October 20, 2022 02:37
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. 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.

7 participants