DOCS-2833 Add script that updates Felix config for OSS#2510
DOCS-2833 Add script that updates Felix config for OSS#2510ctauchen wants to merge 2 commits intotigera:mainfrom
Conversation
✅ Deploy Preview succeeded!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for calico-docs-preview-next ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Adds a new helper script to keep the Felix configuration parameter data in this docs repo synchronized with projectcalico/calico for master and versioned OSS branches (3.30+).
Changes:
- Introduces
scripts/felix-config-update.shto downloadfelix/docs/config-params.jsonfrom upstream Calico. - Updates the unversioned (master) FelixConfig data file and iterates through
calico_versions.jsonto update versioned docs, skipping 3.29.
ctauchen
force-pushed
the
felix-update-script-oss
branch
2 times, most recently
from
f5c37f3 to
6eb2d7b
Compare
|
How does this script fit into your process? Do you run it regularly? Presumably the intent is that you run it and use the changes it makes to raise a PR? The script itself looks fine. |
For now, the idea is just to have this available to run manually for quick and accurate updates. So yes, run and raise a PR. I'd probably aim to run it at release time. Next steps are to include other products and fold into proper automation. But that was beyond the scope for the moment. |
scripts/felix-config-update.sh
Outdated
| @@ -0,0 +1,65 @@ | |||
| #!/bin/bash | |||
|
|
|||
| set -e/-u/pipefail | |||
There was a problem hiding this comment.
I think copilot meant that you should add at least one of the "-e/-u/pipefail" options. The actual syntax would be set -eu -o pipefail
There was a problem hiding this comment.
Yeah, I realized that after when i couldn't run the script! That's changed now.
scripts/felix-config-update.sh
Outdated
| exit 1 | ||
| fi | ||
|
|
||
| curl -o "$LOCAL_PATH" "$REMOTE_URL" |
There was a problem hiding this comment.
I always add -fsSL to my curl invocations.
-fcauses it to return an error code if it gets an HTTP error (so the script ends)-scauses it not to print the (usually pointless) progress indicator (silent)-Sshows (prints out) the error if one happened-Lfollows HTTP redirects instead of saving the redirect response
Long form is --fail --silent --show-error --location
IMHO all of these options should always be used whenever using curl in a shell script, as it catches a lot of errors that would otherwise be missed.
There was a problem hiding this comment.
That's great, thanks. Added.
scripts/felix-config-update.sh
Outdated
|
|
||
| # Exclude version 3.29, which came before direct-from-source method. | ||
| if [ "$VERSION" == "3.29" ]; then | ||
| printf "%s\n" "Skipping $VERSION: No action required." |
There was a problem hiding this comment.
You can use echo "blah blah" instead of printf "%s\n" "blah blah" unless you're doing more complex variable printing (like trying to print values as hex or formatting a number in a specific way). Tends to be more readable.
There was a problem hiding this comment.
I got to that after having trouble with newlines. I think I'll leave it for now, but I take the point that it's cleaner with a plain echo statement.
ctauchen
force-pushed
the
felix-update-script-oss
branch
from
6eb2d7b to
0208d65
Compare
ctauchen
force-pushed
the
felix-update-script-oss
branch
from
0208d65 to
72b7d73
Compare
This script updates the FelixConfiguration component with data from projectcalico/calico and tigera/calico-private. DOCS-2833 Add script for updating Felix configs
ctauchen
force-pushed
the
felix-update-script-oss
branch
from
252079d to
264abe1
Compare
danudey
left a comment
danudey
left a comment
There was a problem hiding this comment.
LGTM, but some suggestions that IMHO will make the script a bit easier to read/maintain in the long run.
| @@ -0,0 +1,157 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
There was a problem hiding this comment.
Running shellcheck shows warnings for the two arrays (reading from json via jq) since, potentially, if the input (the version strings) had spaces they would split incorrectly. Since that's almost definitely not going to happen then, for the sake of pleasing the linter, I'd recommend ignoring these results.
| # shellcheck disable=SC2207 | |
| # cleanup() # | ||
| # Internal function triggered on script exit to remove all temp files. # | ||
| # --------------------------------------------------------------------------- # | ||
| cleanup() { |
There was a problem hiding this comment.
Another linter complain: shellcheck thinks cleanup() is unreachable because it doesn't understand traps. Adding a comment here silences these warnings and gives us a clean linter result. Minor, but good to be clean!
| cleanup() { | |
| # shellcheck disable=SC2317 | |
| cleanup() { |
| printf "%s\n\n" "Begin processing $VERSION" | ||
| printf "%s\n" "Target path: $LOCAL_PATH" | ||
| printf "%s\n" "Source URL: $REMOTE_URL" |
There was a problem hiding this comment.
Might be cleaner to replace printf here. This also lets us see in the script how the lines will be laid out in the output (with an obvious blank line).
| printf "%s\n\n" "Begin processing $VERSION" | |
| printf "%s\n" "Target path: $LOCAL_PATH" | |
| printf "%s\n" "Source URL: $REMOTE_URL" | |
| echo "Begin processing ${VERSION}" | |
| echo | |
| echo "Target path: ${LOCAL_PATH}" | |
| echo "Source URL: ${REMOTE_URL}" |
| fi | ||
|
|
||
| mv "$tmpfile" "$LOCAL_PATH" | ||
| printf "%s\n\n\n" "Finished processing $VERSION." |
There was a problem hiding this comment.
echo -e will interpret escape sequences like newlines, letting us clean this up a bit.
| printf "%s\n\n\n" "Finished processing $VERSION." | |
| echo -e "Finished processing ${VERSION}.\n\n" |
| REMOTE_URL="https://raw.githubusercontent.com/projectcalico/calico/refs/heads/release-v${VERSION}/felix/docs/config-params.json" | ||
|
|
||
| if [[ "$VERSION" == "3.29" ]]; then | ||
| printf "%s\n" "Skipping $VERSION: No action required." |
There was a problem hiding this comment.
| printf "%s\n" "Skipping $VERSION: No action required." | |
| echo "Skipping ${VERSION}: No action required." |
| REMOTE_URL="https://api.github.com/repos/tigera/calico-private/contents/felix/docs/config-params.json?ref=${branch}" | ||
|
|
||
| if [[ "$VERSION" == "3.20-2" ]]; then | ||
| printf "%s\n" "Skipping $VERSION: No action required." |
There was a problem hiding this comment.
| printf "%s\n" "Skipping $VERSION: No action required." | |
| echo "Skipping ${VERSION}: No action required." |
| fi | ||
|
|
||
| printf "OSS Versions: %s\n" "${VERSIONS_OSS[*]}" | ||
| printf "CE Versions: %s\n\n" "${VERSIONS_CE[*]}" |
There was a problem hiding this comment.
| printf "CE Versions: %s\n\n" "${VERSIONS_CE[*]}" | |
| echo "OSS Versions: ${VERSIONS_OSS[*]}" | |
| echo "CE Versions: ${VERSIONS_CE[*]}" | |
| echo |
| update_master_CE | ||
| update_versions_CE VERSIONS_CE | ||
|
|
||
| printf "%s\n" "All updates are complete." |
There was a problem hiding this comment.
| printf "%s\n" "All updates are complete." | |
| echo "All updates are complete." |
3 participants
This script updates the FelixConfiguration component with data the OSS and CE code repositories. A new makefile target makes it easy to run with
make update-felix-config.DOCS-2833
Product Version(s):
Issue:
Link to docs preview:
SME review:
DOCS review:
Additional information:
Merge checklist: