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

fix: correct update script to support version argument again #2156

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Oct 22, 2024

We keep getting key updates from the update script for unchanged node versions, even when the script passes e.g. ./update.sh 22. This is due to a regression in #1307 where the get_versions function no longer looked at its arguments for which version directories to load.

I also got ./update.sh: line 240: pids[@]: unbound variable, so I moved its declaration.

image

I also couldn't resist Object.keys() -> Object.entires() since we were accessing the values in the loop


This should remove the need for PRs such as #2151, as the script should just update the versions with an actual node version change:

const { stdout } = await exec(`./update.sh ${newVersions[version].isSecurityRelease ? "-s " : ""}${version}`);

@SimenB SimenB requested a review from a team October 22, 2024 07:30
@@ -137,12 +137,16 @@ function get_config() {
#
# The result is a list of valid versions.
function get_versions() {
shift
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm really bad as (ba)sh scripting, so there's probably a better solution to this. It works, tho - both with and without the version arguments

@SimenB
Copy link
Member Author

SimenB commented Oct 22, 2024

Not sure how to get around the shellcheck violation. We do call it with arguments (from a different file)

IFS=' ' read -ra update_versions <<< "$(get_versions . "${versions_arg[@]:-}")"

@SimenB
Copy link
Member Author

SimenB commented Oct 30, 2024

@nodejs/docker PTAL

@PeterDaveHello
Copy link
Member

I'll need a bit of time to go through this.

By the way, just wondering if we should consider using AI tools to assist with code reviews and suggestions? I've been trying out https://coderabbit.ai/ recently and found it quite helpful. It provides some valuable suggestions and even catches some errors. Could improve the efficiency for such a busy volunteer team like us 😆

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 3 changed files in this pull request and generated no suggestions.

Files not reviewed (2)
  • functions.sh: Language not supported
  • update.sh: Language not supported
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants