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

Implement a helper function to check minimum binary versions. #1122

Merged
merged 6 commits into from
May 18, 2022
Merged

Implement a helper function to check minimum binary versions. #1122

merged 6 commits into from
May 18, 2022

Conversation

hongil0316
Copy link
Contributor

@hongil0316 hongil0316 commented May 18, 2022

The overall approach is to run a shell command to extract version string:

  • run a shell command (e.g., docker --version)
  • use regex string matcher to extract the exact version string

Right now, it only supports docker, packer, and terraform and the caller need to ensure that those binary exists in the working directory. If we want to support other binaries in the future that potentially has different ways to extract the version string, we can branch it out in the main method e.g., :

version, err
switch binary: 
case Docker, Packer, Terrafor: 
  version, err = getVersionWithShellCommand(t, params)
case ...:
  version, err = getVersionX(t, params)

// other remaining code

TODO: explore ways to mock shell.RunCommandAndGetOutputE so we can test handling different potential output from the shell command end to end.

Fixes #566

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

Looks like a great start! I had a bunch of nits and comments here but they are mostly about style. The core logic seems reasonable. I think this is usable for most cases.

However, to be feature complete, we really want to support flexible version syntax like that supported by Terraform (e.g., being able to say ~> 1.0.0 to limit to > 1.0, < 1.1). Can you look into implementing that?

modules/version-checker/version_checker.go Show resolved Hide resolved
modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rhoboat rhoboat left a comment

Choose a reason for hiding this comment

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

Left some nits! Looks like a great start.

modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

Updates LGTM! I think we are getting closer! I had one more UX nit, but everything else looks good.

The one other meta comment I have is that we should probably have integration tests for each of the three binaries. You already have one for terraform, but would be nice to have one for docker and packer as well. The integration tests will make sure that: (a) the regex works for the three main binaries, (b) the --version flag works for each of the three main binaries

You can pretty much assume all three are setup correctly in the test environment, since terratest is mostly an integration testing library that runs the binaries (e.g., docker package calls out to docker using shell.RunCommand).

modules/version-checker/version_checker.go Outdated Show resolved Hide resolved
@yorinasub17
Copy link
Contributor

yorinasub17 commented May 18, 2022

Thanks for adding the integration tests! I had a clarification for my other suggestion though.

Let me kick off a regression build on this branch since I believe it's ready to go: https://app.circleci.com/pipelines/github/gruntwork-io/terratest/991/workflows/05224958-94f1-4486-a724-adba86f34078

@yorinasub17
Copy link
Contributor

Build failed, but the test failure is unrelated to this change. I have to do one more final pass before approving, but I think this is good to go!

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

Implementation LGTM! Thanks for your contribution! Going to merge and release this now.

@yorinasub17 yorinasub17 merged commit 3f80236 into gruntwork-io:master May 18, 2022
@hongil0316 hongil0316 changed the title Implement a helper funciton to check minimum binary versions. Implement a helper function to check minimum binary versions. May 18, 2022
},
}

for _, tc := range tests {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note that these tests are not running in parallel, unless I'm mistaken. However, they're quick and aren't doing anything like spinning up infrastructure, so running them serially is fine!

If the extractVersionFromShellCommandOutput could become a long-running function it might be worth parallelizing. (Same with the TestCheckVersionConstraint and TestCheckVersionEndToEnd tests.)

For reference, to run them in parallel, you'd need to use t.Run, as in:

for _, tc := range tests {
    t.Run(tc.name, func(t *testing.T) {
        tc := tc
        // run the test here...
    })
}

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.

Feature Request: Packer/Terraform/Docker/Other? Minimum Version Checks
4 participants