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

.*: add verify script for go directive changes #267

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ all:

test:
go test -race ./...
./hack/verify-examples.sh
go -C v2 test -race ./...
(cd v2 && ./hack/verify-examples.sh)

# We verify for the maximum version of the go directive as 1.20
# here because the oldest go directive that exists on our supported
# release branches in k/k is 1.20.
verify:
./hack/verify-examples.sh
./hack/verify-go-directive.sh 1.20
57 changes: 57 additions & 0 deletions hack/verify-go-directive.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
#!/usr/bin/env bash

# Copyright 2024 The Kubernetes Authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

set -o errexit
set -o nounset
set -o pipefail

function usage {
local script="$(basename $0)"

echo >&2 "Usage: ${script} <maximum go directive>
This script should be run at the root of a module.

Compare the go directive in the local working copy's go.mod
to the specified maximum version it can be. Versions provided
here are of the form 1.x.y, without the 'go' prefix.
Examples:
${script} 1.20
${script} 1.21.6
"
exit 1
}

max="$1"
# If max is empty, print usage and error
if [[ -z "${max}" ]]; then
usage;
fi

# Don't specify the version with the go prefix, just 1.x.y will do.
if [[ ! "${max}" =~ ^[0-9]\.[0-9]+(\.[0-9]+)?$ ]]; then
usage
fi

current=$(awk '/^go / {print $2;}' go.mod)
if [[ -z "${current}" ]]; then
echo >&2 "FAIL: could not get value of go directive from go.mod"
exit 1
fi

if ! printf '%s\n' "${current}" "${max}" | sort --check=silent --version-sort; then
Copy link
Member

Choose a reason for hiding this comment

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

Why not test for exact equal? We don't want it to go backwards either, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to understand this a little better, would this serve as a sanity check in general? Because when we bump deps, the go directive is either going to stay the same or its going to increase, I don't think it can go backwards iiuc.

Copy link
Member

Choose a reason for hiding this comment

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

What I meant is that this is obvious:

if [[ "${current}" != "${max}" ]]; then
    echo >&2 "FAIL: current Go directive ${current} must be ${max}"
fi

If I understand this code, you would allow "max" to be 1.20.1 and "current" to be 1.20 - why would we do that? I feel like it's too clever -- don't we ALWAYS want an exact match?

The goal here is that the verify script is not going to be changed "accidentally" (by tooling), so it's an effective (but simple) cross-check.

Copy link
Contributor Author

@MadhavJivrajani MadhavJivrajani Apr 4, 2024

Choose a reason for hiding this comment

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

I understand now, thanks for laying it out @thockin!

Edit: @thockin, the go directive in the go.mod here is 1.13, do we want to bump that because only then will this work

Copy link
Member

Choose a reason for hiding this comment

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

Uggh, after making my point, you clever showed me I was being an idiot. This is not "locking" the version, it really is upper-bounding it. As soon as we add support for type-params, 1.13 will have to bump up, I guess?

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, "upper-bounding" is a nice way to put it. Thanks! <3

As soon as we add support for type-params, 1.13 will have to bump up, I guess?

Yeah, that sounds about right!

echo >&2 "FAIL: current Go directive ${current} is greater than ${max}"
exit 1
fi