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

main: Add -version #444

Merged
merged 1 commit into from
Oct 16, 2018
Merged

main: Add -version #444

merged 1 commit into from
Oct 16, 2018

Conversation

wking
Copy link
Contributor

@wking wking commented Oct 12, 2018

Give callers an easy way to extract both the plugin version and the dependency versions. With this commit:

$ ./terraform-provider-libvirt -version
./terraform-provider-libvirt 48adac309dc181faad7ccd87d52d5e004d5798ec-dirty
Compiled against library: libvirt 3.9.0
Using library: libvirt 3.9.0
Running hypervisor: QEMU 2.9.0
Running against daemon: 3.9.0

The details are very similar to:

$ virsh version --daemon
Compiled against library: libvirt 3.9.0
Using library: libvirt 3.9.0
Using API: QEMU 3.9.0
Running hypervisor: QEMU 2.9.0
Running against daemon: 3.9.0

except the Go bindings do not currently expose the API version. I've submitted a patch to libvirt-go adding ParseVersion there. But until then, carrying our own parseVersion shouldn't be too much trouble.

I hunted around for a way to expose this in Terraform. Currently:

$ cat main.tf
provider "libvirt" {
  uri = "qemu:///system"
}
$ terraform version
Terraform v0.11.8
+ provider.libvirt (unversioned)

but the RPC API is not particularly clear to me, and the versions there may be extracted from plugin filenames and not from RPC calls.

@wking
Copy link
Contributor Author

wking commented Oct 12, 2018

The Travis error was:

$ go get github.com/golang/lint/golint
package github.com/golang/lint/golint: code in directory /home/travis/gopath/src/github.com/golang/lint/golint expects import "golang.org/x/lint/golint"

The command "go get github.com/golang/lint/golint" failed and exited with 1 during .

which sounds like a flake. Is there a way I can kick the tests without pushing a commit with a different hash?

@dmacvicar
Copy link
Owner

You just click "Restart build" on the travis page for the build. I just did it for you.

main.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@MalloZup MalloZup left a comment

Choose a reason for hiding this comment

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

i tend to like the PR but we need to consider 2 cases , and maybe this can add complexity (connecting to remote server this add the complexity imho

  1. in case we provide the plugin as rpm/deb
  2. in case we have a remote kvm-server and we need to take the version ( libvirt-go version and the hyperv version)

Mostly in case 2 would need a sort of ssh-mechanism to go to the host..

Imho we could add an helper script not in main for retrieving the version.

We might also investigate how other Provider upstream do it ( AWS etc)

@wking nice to cu here again 👍 😸 🌹

main.go Show resolved Hide resolved
@MalloZup
Copy link
Collaborator

i think we can retrieve all teh values with URI var.

This change will not be backward compatible with other Version of the plugins but is imho a good thing to have ! thx

@wking
Copy link
Contributor Author

wking commented Oct 12, 2018

  1. in case we provide the plugin as rpm/deb

This is even easier, since we'll get the commit information built in. As it stands, folks who go get the provider (like this) will need to go back to the Git repo manually to retrieve this info.

  1. in case we have a remote kvm-server and we need to take the version ( libvirt-go version and the hyperv version)

The environment variable discussed here should address this.

Mostly in case 2 would need a sort of ssh-mechanism to go to the host..

I don't think we need to go beyond an environment variable for the URI. If someone needs to SSH to libvirtd, they can use SSH to tunnel a port and then set the URI to the local side of the tunnel.

Imho we could add an helper script not in main for retrieving the version.

This would allow for drift between the two binaries. I'd much rather stick to a single binary.

We might also investigate how other Provider upstream do it ( AWS etc)

As I mentioned in my initial comment, I'm not aware of a way for Terraform to collect info like this over RPC. I think it would be awesome if that existed, and it already may, but either way, this integration is beyond me ;).

I dunno if the other providers will have much guidance, since they likely link statically (so no need to wonder "what library version were we dynamically linked to?"). And there's only the one AWS, or at least "which AWS?" does not seem to be configurable, so querying libvirt doesn't seem to have an analog. Maybe other providers have guidance, but I'm content to model virsh, as I've done here. It will be easy enough to shift this around later if we come across an alternative approach to follow.

@wking
Copy link
Contributor Author

wking commented Oct 12, 2018

With the default URI resolved, I think this is good to go again. If there are any remaining issues that I need to address, please let me know.

@MalloZup
Copy link
Collaborator

@wking yop I agree with you there ! I wll check once I have time for that

@wking
Copy link
Contributor Author

wking commented Oct 12, 2018

I've pushed #445 with a fix for the Travis golint issue.

Copy link
Owner

@dmacvicar dmacvicar left a comment

Choose a reason for hiding this comment

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

@MalloZup does LDFLAGS += -X main.version=$$(git describe --always --abbrev=40 --dirty) work when we build with the tarball generated by _service?

@MalloZup
Copy link
Collaborator

Yop good point, I need to check.
@wking would be also awesome that you create a test for this feature, ( I thing shoud be possible)

@wking
Copy link
Contributor Author

wking commented Oct 14, 2018

@wking would be also awesome that you create a test for this feature

Something like:

make build
test "$(./terraform-provider-libvirt -version | head -n1 | cut -c,50)" = "./terraform-provider-libvirt v"

in Travis? (I just guessed for the cut, it probably needs work to strip off the mutable parts of the version). I don't understand @dmacvicar's _service reference.

@MalloZup
Copy link
Collaborator

MalloZup commented Oct 14, 2018

I would have a unit test for the printversion function, especially because if we will refactor in future the name of the var which depends, we mighr break this feature, so better a sinple test. Foe @dmacvicar the service file is used for build thea pkg so feel free to ignore it , we need to check, but is more release pkg task .

For.typos sorry , my hand are bigger the mobile phone keyboard 😸

@dmacvicar
Copy link
Owner

@wking we build here:

https://build.opensuse.org/package/show/systemsmanagement:terraform/terraform-provider-libvirt

And while the tarball is generated from git, it currently does not include the .git subdirectory

@wking
Copy link
Contributor Author

wking commented Oct 14, 2018

terraform-provider-libvirt.spec has:

Version: 0.0

I dunno what's going on there. But _service:set_version:terraform-provider-libvirt.spec has:

Version: 0.5.0

It just needs to call go build with -ldflags "-X main.version=v${VERSION}". Is that something the spec can be updated to do?

@dmacvicar
Copy link
Owner

_service runs a sequence of services: checkout git, create a tarball, and then set_version service updates the version in the _service file based on the git revision/parent tag.

What we would need to do is to keep the last commit of .git inside the tarball, but I am not sure how to do that. I am sure others have done it already. Better to ask internally inside SUSE, somebody would know.

@dmacvicar
Copy link
Owner

dmacvicar commented Oct 15, 2018

Oh no, it is even easier, we just need to do:

LDFLAGS += -X main.version=%{version}

in the spec files, as the service already replaced the version in the spec, and this one is available as a macro. It would not be the exact version format as if somebody runs make from the command line, but I'd prefer to adapt the later to follow the version format of the spec file.

@dmacvicar
Copy link
Owner

I did some experiments and -X main.version can be set independently of the variable existing on main. So we can change the spec file to set main.version before merging this PR.

@MalloZup
Copy link
Collaborator

@dmacvicar nice. imho would be safe also to add a unit-test before merging it.

I don\t feel confortable adding features (especially command-line args) untested, especially if we depend for variables used/definitd in other files.

Awesome 👍

@MalloZup
Copy link
Collaborator

MalloZup commented Oct 15, 2018

and keeping the discussion further, i was thinking to put the SPEC files here in github, so contributors could modify if needed. ( we use the same mechanism uyuni)

Could be this douable? what is your pov?

@wking
Copy link
Contributor Author

wking commented Oct 15, 2018

I've submitted a patch to libvirt-go adding ParseVersion there. But until then, carrying our own parseVersion shouldn't be too much trouble.

The patch finally made it through moderation and landed on the list here. I'll follow-up here if/when that lands.

LDFLAGS += -X main.version=%{version}

You'll want the leading v (see here). For example:

$ git checkout v0.5.0
$ git describe --always --abbrev=40 --dirty
48adac309dc181faad7ccd87d52d5e004d5798ec

Oh, wait. Are you not signing your tags (or not giving them messages)? If all the tags are lightweight, we'll need to use:

$ git describe --always --abbrev=40 --dirty --tags
v0.5.0

So I think you want a v prefix in your spec files to match folks using the Makefile logic directly.

And can we rely on future tags being signed (or having a message)? Or do I need to add --tags here?

Give callers an easy way to extract both the plugin version and the
dependency versions.  With this commit:

  $ ./terraform-provider-libvirt -version
  ./terraform-provider-libvirt 48adac3-dirty
  Compiled against library: libvirt 3.9.0
  Using library: libvirt 3.9.0
  Running hypervisor: QEMU 2.9.0
  Running against daemon: 3.9.0

The details are very similar to:

  $ virsh version --daemon
  Compiled against library: libvirt 3.9.0
  Using library: libvirt 3.9.0
  Using API: QEMU 3.9.0
  Running hypervisor: QEMU 2.9.0
  Running against daemon: 3.9.0

except the Go bindings do not currently expose the API version [1].
I've submitted a patch to libvirt-go adding ParseVersion there.  But
until then, carrying our own parseVersion shouldn't be too much
trouble.

Passing an empty string to NewConnect ends up passing a nil pointer
through to virConnect [2], which will give us libvirt's internal
default URI logic [3,4].

I hunted around for a way to expose the version information in
Terraform.  Currently:

  $ cat main.tf
  provider "libvirt" {
    uri = "qemu:///system"
  }
  $ terraform version
  Terraform v0.11.8
  + provider.libvirt (unversioned)

but the RPC API is not particularly clear to me, and the versions
there may be extracted from plugin filenames and not from RPC calls.

[1]: https://libvirt.org/git/?p=libvirt-go.git;a=blob;f=connect.go;h=8cc7cc771f7d258400175df47fcc8b3779ef4930;hb=9c5bdce3c18faad94cb383e7ec42f5122a8c7fa1#l313
[2]: https://libvirt.org/git/?p=libvirt-go.git;a=blob;f=connect.go;h=8cc7cc771f7d258400175df47fcc8b3779ef4930;hb=9c5bdce3c18faad94cb383e7ec42f5122a8c7fa1#l322
[3]: https://libvirt.org/html/libvirt-libvirt-host.html#virConnectOpen
[4]: https://libvirt.org/uri.html#URI_default
@wking
Copy link
Contributor Author

wking commented Oct 15, 2018

I've pushed ef77653 -> 91d0e45 rebasing onto the current master and adding a unit test for printVersion.

Copy link
Collaborator

@MalloZup MalloZup left a comment

Choose a reason for hiding this comment

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

💘

@MalloZup MalloZup mentioned this pull request Oct 16, 2018
@MalloZup MalloZup merged commit fe57374 into dmacvicar:master Oct 16, 2018
@dmacvicar
Copy link
Owner

I am having problems updating the .spec file.

For non-SUSE we are fine, as we build with plain go build, which I updated to:

go build -ldflags "-X main.version=%{version}" .

But for SUSE we use the %gobuild macro, and I haven't figured out how to add arguments to it.

@MalloZup
Copy link
Collaborator

@dmacvicar we might don\t use the macro?

wking added a commit to wking/terraform-provider-libvirt that referenced this pull request Oct 20, 2018
Catching up with 91d0e45 (main: Add -version, 2018-10-11, dmacvicar#444).
MalloZup pushed a commit to MalloZup/terraform-provider-libvirt that referenced this pull request Oct 26, 2018
Catching up with 91d0e45 (main: Add -version, 2018-10-11, dmacvicar#444).
@wking wking deleted the version-option branch October 30, 2018 21:34
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.

3 participants