search

kubernetes / contributor-site

Code for kubernetes.dev
https://www.kubernetes.dev
Apache License 2.0
67 stars 149 forks source link

Add page to list all KEPs #324

Closed palnabarun closed 1 year ago

palnabarun commented 2 years ago

Part of https://github.com/kubernetes/enhancements/issues/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.

palnabarun commented 2 years ago

/assign @sftim @mrbobbytables

(since you reviewed #222)

sftim commented 2 years ago

Some of the approach in https://github.com/kubernetes/website/pull/35228 might be useful for generating a list of KEPs.

palnabarun commented 2 years ago

@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 commented 2 years ago

I think this is OK.

/hold

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

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

sftim commented 2 years ago

/lgtm

Can we skip KEPS with a KEP ID < 1?

palnabarun commented 2 years ago

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 commented 2 years ago

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".

palnabarun commented 2 years ago

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 commented 2 years ago

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

sftim commented 2 years ago

Try title rather than humanize @palnabarun

palnabarun commented 2 years ago

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.

dims commented 2 years ago

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

sftim commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

sftim commented 2 years ago

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 commented 2 years ago

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

palnabarun commented 2 years ago

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

k8s-ci-robot commented 1 year ago

[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: - ~~[OWNERS](https://github.com/kubernetes/contributor-site/blob/master/OWNERS)~~ [mrbobbytables] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
sftim commented 1 year ago

Let's ship it

/hold cancel