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

Integration test for docker buildx version #1985

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

jsternberg
Copy link
Collaborator

An integration test for docker buildx version has been created. The
integration test checks that there is one line output, the output is
composed of three sections, and that these sections could feasibly be
the package path, version, and revision information.

The intention of the checks is to find obvious errors in the output like
the package path not existing or the version and revision being swapped.
It is not intended to assert that these values must be certain values
because it is assumed these values may vary depending on the build
process for buildx.

Related to #1857.

@jsternberg jsternberg force-pushed the integration-test/version branch 4 times, most recently from 3392ff0 to 8eebc28 Compare August 3, 2023 02:55
tests/version.go Outdated
// Second component should be a version.
// This defaults to something that's still compatible
// with semver.
require.True(t, semver.IsValid(components[1]), "Second component was not valid semver: %+v", components[1])
Copy link
Member

Choose a reason for hiding this comment

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

It seems to make sense that version is not semver when running tests. We use git describe to set the version:

git describe --match 'v[0-9]*' --dirty='.m' --always --tags

And looking at the GHA workflow:

-
name: Checkout
uses: actions/checkout@v3

We don't fetch tags so only commit sha will be returned. It should work if you set:

      -
        name: Checkout
        uses: actions/checkout@v3
        with:
          fetch-depth: 0

See https://github.com/actions/checkout/#usage

# Number of commits to fetch. 0 indicates all history for all branches and tags.
# Default: 1
fetch-depth: ''

go.mod Outdated
@@ -39,6 +39,7 @@ require (
github.com/zclconf/go-cty v1.10.0
go.opentelemetry.io/otel v1.14.0
go.opentelemetry.io/otel/trace v1.14.0
golang.org/x/mod v0.12.0
Copy link
Member

Choose a reason for hiding this comment

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

If we don't need to bump to v0.12.0, can you just keep v0.9.0? But if needed then can you do the vendoring in a dedicated commit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea I can throw this back down to v0.9.0. I figured it was likely at v0.9.0 because it was an implicit dependency, but it's likely better to just do any version upgrades as a different PR anyway.

@thaJeztah
Copy link
Member

thaJeztah commented Aug 3, 2023

Perhaps we should check that the last element is the version, and not necessarily that there's 3 components (and exactly one line); https://www.gnu.org/prep/standards/html_node/_002d_002dversion.html#g_t_002d_002dversion

ideally the middle part (packaging info) would be between brackets, but at least convention / guidelines (see above, although admitted that is for --version) is the last element of the first line to be the version.

The output may have multiple lines, which can be used to print (e.g.) licensing info / notices

@jedevc jedevc mentioned this pull request Aug 3, 2023
35 tasks
@jedevc
Copy link
Collaborator

jedevc commented Aug 3, 2023

@jsternberg just a small note - I tend to avoid trying to include PR/issue numbers in commits, just since GitHub chooses to then spam these quite heavily into those issue descriptions 😢 (see in #1857)

@jsternberg jsternberg force-pushed the integration-test/version branch from 8eebc28 to e86811e Compare August 3, 2023 13:49
@jsternberg
Copy link
Collaborator Author

@jedevc yea I was wondering about that. I'll remove it. The merge commit should have the github pull request number which could then get traced back to the original PR and I can keep the issue description in the PR itself so it doesn't spam.

@jsternberg
Copy link
Collaborator Author

@thaJeztah I think changing the check to follow the standard set by GNU would be fine although it seems our current output prints the revision last when it's available so we'd probably just need to check "is there something that looks like a version in the first line?"

I do think there's some value in making the check a bit more strict though. There may be people potentially using the output for some reason and, while the output is likely not listed as stable, we shouldn't change it accidentally. I also think there's some value in the test breaking if the output changes drastically and then fixing the test to compensate.

I do also think that testing that the output is a single line might be a bit aggressive. I think I should remove that.

@jsternberg jsternberg force-pushed the integration-test/version branch 2 times, most recently from adbb4b1 to 6e1710b Compare August 3, 2023 14:28
An integration test for `docker buildx version` has been created. The
integration test checks that there is one line output, the output is
composed of three sections, and that these sections could feasibly be
the package path, version, and revision information.

The intention of the checks is to find obvious errors in the output like
the package path not existing or the version and revision being swapped.
It is not intended to assert that these values must be certain values
because it is assumed these values may vary depending on the build
process for buildx.

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
@jsternberg jsternberg force-pushed the integration-test/version branch from 6e1710b to 1d12c1f Compare August 3, 2023 14:51
@jedevc jedevc merged commit 50fbdd8 into docker:master Aug 4, 2023
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.

4 participants