Skip to content

Conversation

Rishab87
Copy link
Contributor

@Rishab87 Rishab87 commented Sep 7, 2025

What this PR does / why we need it:
This PR automates the process of updating the data.yaml file. To run specify the new version like this:

./update-data-yaml.sh v2.18.0

For each new version, it fetches the latest Kubernetes releases (we keep the k8s version one behind the latest release), adds the new compatibility mapping, and prunes the oldest entry. This ensures our compatibility data is kept current automatically with each new release.

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
does not change

Which issue(s) this PR fixes: (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged)
Related to #2756

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 7, 2025
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If kube-state-metrics contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Rishab87
Once this PR has been reviewed and has the lgtm label, please assign catherinef-dev for approval. For more information see the 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

@github-project-automation github-project-automation bot moved this to Needs Triage in SIG Instrumentation Sep 7, 2025
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 7, 2025
Comment on lines 37 to 54
echo "Fetching latest Kubernetes version..."
LATEST_K8S_FULL_VERSION=$(curl --silent --fail "https://api.github.com/repos/kubernetes/kubernetes/releases/latest" | jq -r '.tag_name')

if [ -z "$LATEST_K8S_FULL_VERSION" ] || [ "$LATEST_K8S_FULL_VERSION" == "null" ]; then
echo "Error: Failed to fetch the latest Kubernetes version from GitHub." >&2
exit 1
fi

LATEST_K8S_VERSION=$(echo "${LATEST_K8S_FULL_VERSION}" | sed 's/^v//' | cut -d. -f1,2)
echo "Latest stable Kubernetes version (N): ${LATEST_K8S_VERSION}"

K8S_MAJOR=$(echo "${LATEST_K8S_VERSION}" | cut -d. -f1)
K8S_MINOR=$(echo "${LATEST_K8S_VERSION}" | cut -d. -f2)
PREV_K8S_MINOR=$((K8S_MINOR - 1))
K8S_VERSION_FOR_NEW_RELEASE="${K8S_MAJOR}.${PREV_K8S_MINOR}"
echo "New release ${NEW_VERSION_WITH_V} will be mapped to Kubernetes (N-1): ${K8S_VERSION_FOR_NEW_RELEASE}"
Copy link
Member

@mrueg mrueg Sep 8, 2025

Choose a reason for hiding this comment

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

I think it should check k8s.io/client-go's version in go.mod when bumping, if I understand it correctly. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup right, changed it

@Rishab87 Rishab87 requested a review from mrueg September 8, 2025 14:08

check_command "yq"
check_command "jq"
check_command "curl"
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we don't need curl anymore. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup

Comment on lines 16 to 17
check_command "yq"
check_command "jq"
Copy link
Member

Choose a reason for hiding this comment

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

Can we use github.com/itchyny/gojq/cmd/gojq instead?

Once #2660 is in, it will be used anyway as part of the project. It supports yaml via --yaml-input --yaml-output args.

Copy link
Contributor Author

@Rishab87 Rishab87 Sep 9, 2025

Choose a reason for hiding this comment

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

added this, havent pushed go.mod and go.sum since this is to be merged after that PR

exit 1
fi

CLIENT_GO_FULL_VERSION=$(grep 'k8s.io/client-go' "${GO_MOD_FILE}" | awk '{print $2}')
Copy link
Member

Choose a reason for hiding this comment

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

@Rishab87
Copy link
Contributor Author

Rishab87 commented Sep 9, 2025

@mrueg I've addressed all the reviews, can you please re-review?

@Rishab87 Rishab87 requested a review from mrueg September 9, 2025 14:29
Copy link
Member

@rexagod rexagod left a comment

Choose a reason for hiding this comment

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

Also, generally I'd like to see the more complex and lengthy pipe-based commands here broken down and commented at each step (concisely) so they're easier to maintain.

-FOO=$(a|b|c)
+FOO=$(\
+# what "a" does
+ a|\
+# what "b" does
+ b|\
+# what "c" does
+ c\
+)

Comment on lines 81 to 86
echo "${JSON_DATA}" | gojq -r --arg version "${NEW_VERSION_WITH_V}" --arg k8s "${K8S_VERSION_FOR_NEW_RELEASE}" '
.compat[] | select(.version != $version) | " - kubernetes: \"" + .kubernetes + "\"\n version: " + .version
' >> "${DATA_FILE}"

echo " - kubernetes: \"${K8S_VERSION_FOR_NEW_RELEASE}\"" >> "${DATA_FILE}"
echo " version: ${NEW_VERSION_WITH_V}" >> "${DATA_FILE}"
Copy link
Member

Choose a reason for hiding this comment

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

I maybe off but wouldn't append main to the list as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it's being appended separately below

VERSIONS_LIST=$(echo "${JSON_DATA}" | gojq -r '.compat[] | select(.version != "main") | "\(.version)|\(.kubernetes)"' 2>/dev/null || true)
FULL_VERSIONS_LIST=$(printf "%s\n%s|%s" "${VERSIONS_LIST}" "${NEW_VERSION_WITH_V}" "${K8S_VERSION_FOR_NEW_RELEASE}")

SORTED_VERSIONS=$(echo "${FULL_VERSIONS_LIST}" | grep -v '^$' | sort -t'|' -k1,1 -Vr | head -n "${NUM_RELEASES_TO_KEEP}" | sort -t'|' -k1,1 -V)
Copy link
Member

Choose a reason for hiding this comment

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

Again, I maybe off here since I've not run this, but wouldn't this take into account the current 5 releases, instead of dropping the oldest one, and appending the given release and then main?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it takes into account current 5 releases excluding the main, so it removes the oldest release, adds the new one and updates the main version too if needed

@Rishab87
Copy link
Contributor Author

@rexagod I've made the suggested changes, can you please re-review it? Also I've updated go.sum and go.mod to add gojq as a dependency in this PR, so this PR does not depend on #2660 to get merged

K8S_MINOR=$(echo "${CLIENT_GO_FULL_VERSION}" | cut -d. -f2)

K8S_VERSION_FOR_NEW_RELEASE="1.${K8S_MINOR}"
echo "New release ${NEW_VERSION_WITH_V} will be mapped to Kubernetes (N-1): ${K8S_VERSION_FOR_NEW_RELEASE}"
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean with the n-1 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added that by mistake I was thinking something while writing down this script, removing it.

Copy link
Member

@rexagod rexagod Oct 1, 2025

Choose a reason for hiding this comment

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

I think that's supposed to mean the latest-1 release, relative to the latest KSM release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I think so I added it earlier because I was subtracting one version behind the latest k8s version but later Manuel suggested to take the k8s version from go.mod instead of fetching from github api and subtracting it.

echo "New release ${NEW_VERSION_WITH_V} will be mapped to Kubernetes (N-1): ${K8S_VERSION_FOR_NEW_RELEASE}"


JSON_DATA=$(cat "${DATA_FILE}" | gojq -r --yaml-input '.')
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to use cat or can gojq also read from a file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, gojq can read directly from a file without using cat. I've already removed cat from here

EXISTING_EXACT_MATCH=$(\
echo "${JSON_DATA}" |\
# Query for existing entry with same version and k8s mapping
gojq -r --arg version "${NEW_VERSION_WITH_V}" --arg k8s "${K8S_VERSION_FOR_NEW_RELEASE}" ".compat[] | select(.version == \$version and .kubernetes == \$k8s) | .version // empty"\
Copy link
Member

Choose a reason for hiding this comment

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

You can call gojq via go tool similar to how we do it in the makefile

@Rishab87
Copy link
Contributor Author

@mrueg I've addressed all the reviews, can you please re-review it?

@Rishab87 Rishab87 requested a review from mrueg September 25, 2025 10:56
@Rishab87
Copy link
Contributor Author

@mrueg @rexagod just a follow up on this one

@@ -0,0 +1,152 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

I believe this file isn't executable by default, can you run chmod +x and commit it again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

# Remove empty lines
grep -v '^$' |\
# Sort by version (descending) using version sort
sort -t'|' -k1,1 -Vr |\
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to sort twice here? Could we use tail instead of head?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup you're right, thanks for pointing it out made the changes


SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
REPO_ROOT=$( cd -- "${SCRIPT_DIR}/.." &> /dev/null && pwd )
DATA_FILE="${REPO_ROOT}/data.yaml"
Copy link
Member

Choose a reason for hiding this comment

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

How about maintaining a couple of mock data files (inputs and outputs), and populating them with the various use-cases covered here, to ensure functionality doesn't break over time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah nice idea, added them

@Rishab87
Copy link
Contributor Author

Rishab87 commented Oct 1, 2025

@rexagod I've addressed all your reviews, can you please re-review it?

@Rishab87 Rishab87 requested a review from rexagod October 1, 2025 17:57
@rexagod rexagod moved this from Needs Triage to In Progress in SIG Instrumentation Oct 3, 2025
@Rishab87
Copy link
Contributor Author

Rishab87 commented Oct 7, 2025

@rexagod just a follow up on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants