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

Introduce vCD API version checks #174

Merged
merged 20 commits into from
Apr 10, 2019
Merged

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented Apr 8, 2019

Closes #173

As we already spotted (#173) sometimes even the same API version on vCD has some differences which caused test suite failures. This PR introduces ability to check on API supported versions using Hashicorp's go-version package in a friendly manner: "> 27", "=27", ">= 27", etc. It does not directly allow to match vCD product versions (9.x.x), because there is no API to get them.

Exported API introduces two methods:

  • APIMaxVerIsAPIVCDMaxVersionIs (allows to compare against vCD Maximum supported version and is useful to understand which vCD version is running https://code.vmware.com/doc/preview?id=8072)
  • APICurVerIsAPIClientVersionIs (allows to compare against currently used API version)

Also introduced a configuration option WithAPIVersion(version string) VCDClientOption which allows to easily override API version used

@Didainius Didainius requested review from dataclouder and vbauzys and removed request for dataclouder April 8, 2019 06:55
Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

Overall, nice PR! Some specific feedback below.

Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

Some changes requested. I may ask for more after the tests finish.

// Format: ">= 27.0, < 32.0", ">= 30.0", "= 27.0"
//
// vCD version mapping to API version support https://code.vmware.com/doc/preview?id=8072
func (vdcCli *VCDClient) APIMaxVerIs(versionConstraint string) bool {
Copy link
Contributor

@dataclouder dataclouder Apr 8, 2019

Choose a reason for hiding this comment

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

It should be vcdCli, not vdcCli.
This error is present in many methods through the library. It would be nice to fix all of them. (I may have been part of the misnaming)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. It seemed confusing and now I know why. I will fix it either in the end of this PR (to avoid many altered files in PR) or in a separate one.

Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

Small improvements needed, but overall looks good.

var versionTests = []struct {
version string
boolChecker Checker
isSsupported bool
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: isSsupported should be isSupported

func (vcd *TestVCD) Test_APIClientVersionIs(check *C) {

// Check with currently set version
r := vcd.client.APIClientVersionIs(fmt.Sprintf("= %s", vcd.client.Client.APIVersion))
Copy link
Contributor

Choose a reason for hiding this comment

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

What does r stand for? Wouldn't be better to use a full word?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

r = result. Sorry, it's that same idiom sitting on me I'm changing it.

Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

LGTM!

@Didainius Didainius merged commit d6baa51 into vmware:master Apr 10, 2019
@Didainius Didainius deleted the issue-173-2 branch May 13, 2019 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants