-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 new push-node.sh that uses pre-compiled upstream releases #3825
add new push-node.sh that uses pre-compiled upstream releases #3825
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
/lgtm
IMAGE_NAME="${IMAGE_NAME:-node}" | ||
|
||
# cd to the repo root | ||
REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." &> /dev/null && pwd -P)" |
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.
Can we always assume we will be in a git repo and not running on a downloaded copy of the source? If so, might be slightly safer to use git rev-parse --show-toplevel
.
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.
This approach is safe unless we move the script relative to the repo root, we use it pretty widely in Kubernetes. BASH_SOURCE[0] will be the script invoked and is reliable vs $0 or something like that.
It also works with a downloaded source tarball, for this particular script maybe we don't care, but I prefer using a consistent pattern, and for other scripts some packagers are using tarballs IIRC. (though we currently also have #3645)
This trick is a neat combination of things that are safe versus the more trivial dirname "${BASH_SOURCE}"
- we use a subshell, so we can CD without affecting the current working directory (and we'd want that anyhow to capture the output)
- we use CD +
pwd -P
to portably resolve the real path (versus something like realpath/readlink that are portability nightmares) - we use BASH_SOURCE[0] to specifically get the primary source script in a supported way instead of depending on something like "$0 will probably maybe have it"
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.
(pwd -P
happens to be specified in posix: https://pubs.opengroup.org/onlinepubs/007904875/utilities/pwd.html and that flag forces it to resolve symlinks, but only for the current working directory)
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.
The main downside of this approach is you need to know the levels of nesting to the repo root (/../..
) but that only needs changing if we move it to be more or less nested (like this, the other script is /../../..
), which won't happen often and you're going to catch the first time you run the script after moving it.
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.
(er and BASH_SOURCE
is safe because we specifically use #!/usr/bin/env bash
and bash specifies this, we usually require bash for development scripts in kubernetes (along with gnu coreutils), I use POSIX sh for some portable utilities that I want end users to run, like the local registry script, but it lacks a lot of modern features and is avoided in favor of bash, go, etc otherwise)
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.
... also I think git rev-parse --show-toplevel
doesn't work for git worktree, which is something we're starting to use more in Kubernetes scripts (e.g. for diffing codegen in verify scripts)
... I've spent way too much time hacking on these scripts :^), we should really write more of this down somewhere ...
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
# this script replaces hack/release/build/push-node.sh for Kubernetes v1.31+ |
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 started as a copy, I added VERSION="$1"
and then ripped out all of the kubernetes build and version detection bits in favor of passing this as the kind build node-image
argument)
/retest |
much faster using official upstream release bits, we'll still want to use the old one for <v1.31.0 (to avoid having larger images) but for v1.31.0+ we can switch to this one.
FYI @aojea
tested with
hack/release/push-node.sh v1.31.4
(which pushed togcr.io/k8s-staging-kind/node:v1.31.4@sha256:7ed51d914668aab502b66f2a82d824be627b4bcd3653f3a119f2bc25f49b81b0
)