-
Notifications
You must be signed in to change notification settings - Fork 777
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
base: master
Are you sure you want to change the base?
Add version popout to "Packaged Distributions" table #3842
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
@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. |
The screenshots look good to me. |
Signed-off-by: Mathew Wicks <[email protected]>
Signed-off-by: Mathew Wicks <[email protected]>
Signed-off-by: Mathew Wicks <[email protected]>
Signed-off-by: Mathew Wicks <[email protected]>
27ec59f
to
baecd28
Compare
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 |
/cc @kubeflow/kubeflow-steering-committee |
@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:
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. |
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]>
@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 |
@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. |
There was a problem hiding this 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.”
Signed-off-by: Mathew Wicks <[email protected]>
@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. |
Signed-off-by: Mathew Wicks <[email protected]>
@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. |
Signed-off-by: Mathew Wicks <[email protected]>
@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. |
+1 to the latest updates. |
@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 @@ | |||
{ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 @@ | |||
{ |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: Mathew Wicks <[email protected]>
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:
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)Please note, all of the styling is automatic:
content/en/docs/started/installing-kubeflow/get_new_releases.sh
to cache that version's release date into thecontent/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
NEW
Table Overview
On-Hover ("Latest Version")
On-Hover ("1 from Latest Version")
On-Hover ("3 from Latest Version")