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 version popout to "Packaged Distributions" table #3842

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

thesuperzapper
Copy link
Member

@thesuperzapper thesuperzapper commented Aug 23, 2024

This PR improves the "Packaged Distributions" table by making it clearer how old/new each Kubeflow Version is. Right now, it's very confusing to new users, as they might not know that Kubeflow 1.7 is actually very old (released in March 2023), so they might start the process of adopting a long-abandoned distribution.

The key changes this PR makes are:

  1. In the "Packaged Distributions" table under the "Kubeflow Version" column:
    • Put a hover-able icon that says how long ago that version of Kubeflow was released. (Note, this is the Kubeflow released date, not the release date of the distribution).
    • Highlight distributions which are up to date (within 1 minor version of the latest), and de-emphasize distributions which are 2+ minor versions out of date.
    • Place warning icons beside versions which are very old (2 = warning, 3+ = scull)
  2. Remove the very old link to the historical "conformance program" which was started by google but never continued.

Please note, all of the styling is automatic:

  • when the short-codes for that distribution's version are updated everything will change automatically
  • when we release a new version of Kubeflow, can use the content/en/docs/started/installing-kubeflow/get_new_releases.sh to cache that version's release date into the content/en/docs/started/installing-kubeflow/release-info folder of JSON (the build will fail to remind you if you try referencing a version which it does not know about)

Screenshots

OLD

Screenshot 2024-10-14 at 12 10 41

NEW

Table Overview

Screenshot 2024-10-23 at 11 05 21

On-Hover ("Latest Version")

Screenshot 2024-10-29 at 12 29 50

On-Hover ("1 from Latest Version")

Screenshot 2024-10-29 at 12 29 20

On-Hover ("3 from Latest Version")

Screenshot 2024-10-29 at 12 30 11

Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign james-jwu for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@thesuperzapper
Copy link
Member Author

@jbottum @andreyvelich @kimwnasptd @terrytangyuan @ederign please take a look at this important style update to the "Packaged Distributions" table.

It should help avoid confusion with new users about which distributions are currently supported.
See the preview site here:

@juliusvonkohout
Copy link
Member

The screenshots look good to me.

@thesuperzapper thesuperzapper force-pushed the add-version-popout-to-distribution-table branch from 27ec59f to baecd28 Compare August 28, 2024 17:41
@jbottum
Copy link
Contributor

jbottum commented Sep 3, 2024

Since this impacts how distributions are promoted, my suggestion is that it would be good if a majority of the distribution owners approved this i.e. we need 6 distributions to provide a +1 or lgtm

@terrytangyuan
Copy link
Member

/cc @kubeflow/kubeflow-steering-committee
/hold

Copy link

@terrytangyuan: GitHub didn't allow me to request PR reviews from the following users: kubeflow/kubeflow-steering-committee.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @kubeflow/kubeflow-steering-committee
/hold

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jbottum
Copy link
Contributor

jbottum commented Oct 4, 2024

I personally like the version, but am -1 on the emojis. I propose that this should be implemented if a majority of the distributions are +1.

Signed-off-by: Mathew Wicks <[email protected]>
@thesuperzapper
Copy link
Member Author

@jbottum @terrytangyuan as discussed in the last community call, I have removed the warning icons (see new screenshots), which should make this PR very non-controversial now.

As agreed in that meeting, we will give people a week or so to make comments about the proposed change (I will highlight it again in the community meeting tomorrow) and if we don't hear anything, I think we can go ahead and merge this.

PS: the code will automatically keep everything up to date, so there is no maintenance overhead to this PR

@jbottum
Copy link
Contributor

jbottum commented Oct 15, 2024

@thesuperzapper I am more inclined to +1 without the grey out of older versions but I will differ to the wishes of the majority of the distributions.

Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

Based on the understanding from the community call 2024-10-15, I have some doubt about the use of grey colour, since as well I understand (from the same call) there is not in place an automated conformance-programme, and the rules by which it moves from blue colour to grey colour.
Hence, I wonder what about instead of grey, the table cell in that case is simply rendered as-before.
i.e.:

  • blue pill for "recent" in blue, imho ok
  • do not grey pill
  • on-hover, imho ok

Personally, I find the on-hover very helpful.
I have one change suggestion however, because I think the wording on the last on-hover is a bit "assuming".

“Opinions expressed are my own.”

layouts/shortcodes/kf-version-notice.html Outdated Show resolved Hide resolved
Signed-off-by: Mathew Wicks <[email protected]>
@thesuperzapper
Copy link
Member Author

@andreyvelich I have updated the PR with the changes discussed from the community meeting (all distributions are now blue, and the wording is less strong on the 3+ versions out of date message).

I think you can approve this now.

@jbottum
Copy link
Contributor

jbottum commented Oct 24, 2024

@thesuperzapper thanks for these updates. I am lukewarm to the commentary in the version popups i.e. the 2nd sentences. The 1st sentence already tells the user if the distribution is current. Do we need the 2nd sentence ? Do they need to be different if the distribution is not current ? If not current, then something like...Please check with this distribution's maintainers to understand their support plans.

@thesuperzapper
Copy link
Member Author

@jbottum I have updated the text in the second paragraph so that they all have the same text.

See the updated screenshots above, or deploy preview.

If no one has any disagreements by the next community meeting, we should be good to merge this.

@jbottum
Copy link
Contributor

jbottum commented Oct 30, 2024

+1 to the latest updates.

@jbottum
Copy link
Contributor

jbottum commented Oct 30, 2024

@nvkassus @chensun @james-jwu @yhwang @johnugeorge @nagar-ajay @rimolive @juliusvonkohout @xujinheng @alexeadem please review the proposed presentation of your distribution on the Installing Kubeflow page and provide your +1 or -1 and/or comments. We have put a lot of time/work into this (during the Community calls) and would like to finish up and merge. thanks!

@@ -0,0 +1,6 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to keep JSONs for versions that < 1.6.0 since we don't have any distribution that runs on Kubeflow <=1.6

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just easier for the automated script to get all of them, and it's not like there are that many.

@@ -0,0 +1,6 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

We might need to add information on how to update those JSONs into release Handbook: https://github.com/kubeflow/community/blob/master/releases/handbook.md
cc @kubeflow/release-team

@thesuperzapper Would it be easier to have single JSON with the list of Kubeflow versions and publish dates ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@andreyvelich you can add that if you like, but the system is designed to fail the website build when a distribution tries to claim a version that it does not know about.

The error will tell the person making that PR to run the script at ./content/en/docs/started/installing-kubeflow/get_new_releases.sh.

That script uses the releases on the kubeflow/manfistes repo to get the versions and determine the release dates.

Copy link
Member Author

Choose a reason for hiding this comment

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

But you have reminded me to run the script on this PR as we just released 1.9.1, I will do that now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants