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

Test $::architecture also against values used by Debian/Ubuntu #2

Merged
merged 2 commits into from Mar 10, 2016

Conversation

amosshapira
Copy link
Contributor

On Debian based systems, $::architectrue is converted to amd64 and fails the test.
$::hardwareisa seems to be more consistent across the board.
Ref: http://stackoverflow.com/a/12696802/164137
Both are considered 'Legacy Facts' which should be changed to structured facts in the future.

@mmarseglia mmarseglia added the bug label Mar 10, 2016
@mmarseglia
Copy link
Owner

@amosshapira thanks for the PR! I booted a couple VMs and you're right about architecture being different. I was using CentOS to test and didn't pick up on this.

I just booted a couple Debian 7.x VMs and saw that hardwareisa is unknown in the VM. Here's the facter output from the puppetlabs/debian 7.8 32-bit and 64-bit vagrant boxes, respectively.

puppetlabs/debian-7.8-32-nocm

architecture => i386
...
hardwareisa => unknown
hardwaremodel => i686
...
lsbdistdescription => Debian GNU/Linux 7.8 (wheezy)
...
virtual => vmware

puppetlabs/debian-7.8-64-nocm

architecture => amd64
...
hardwareisa => unknown
hardwaremodel => x86_64
lsbdistdescription => Debian GNU/Linux 7.8 (wheezy)
virtual => vmware
vagrant@localhost:~$ uname -a
Linux localhost 3.2.0-4-amd64 #1 SMP Debian 3.2.68-1+deb7u2 x86_64 GNU/Linux

Interestingly hardwaremodel is populated with x86_64.

One use case is to have vmware workstation running inside a VM. I actually do this on a virtualized Jenkins node to automate builds w/ packer. If we switch to hardwareisa then installing vmware workstation inside a VM would fail.

Maybe the best way around this is to use the fact architecture and throw in a conditional for amd64.

if ($architecture == x86_64) or ($architecture == amd64) {

Or switch to the new fact, os.architecture?

Thoughts?

@amosshapira
Copy link
Contributor Author

It appears to be an issue specific with Debian 7 - what Facter version do you use?

I'd love to switch to the new fact but got the impression that it's only supported in much later versions of Facter and therefore it'll break compatibility with most Facter installations out there.

I was about to propose a similar fix before I found $::hardwareisa:

if ($::architecture in ['x86_64', 'amd64']) {

I suppose that's the safest way for now. I'll test this and update my PR.

@mmarseglia
Copy link
Owner

I'm using open source puppet from Debian 7's apt repository.

vagrant@localhost:~$ facter -v
1.6.10

@amosshapira
Copy link
Contributor Author

I'm not sure how to update the tests.

@mmarseglia
Copy link
Owner

Looks like there's a typo on line 83 of init.pp.

@amosshapira
Copy link
Contributor Author

OK, all fixed (and commits squashed).

@amosshapira amosshapira changed the title Use $::hardwareisa instead of $::architecture Test $::architecture also against values used by Debian/Ubuntu Mar 10, 2016
@amosshapira
Copy link
Contributor Author

There is still the issue that the downloads fail, probably because $::architecture is used in the URL but testing manually with wget with x86_64 also fails. What's supposed to be the full URL?

In http://www.hackerway.ch/2015/09/09/how-to-install-vmware-workstation-12-on-ubuntu-14-04-lts/ I found that it just uses a static URL of https://www.vmware.com/go/tryworkstation-linux-64

EDITED - the URL above redirect to /software/wkst/file/VMware-Workstation-Full-12.1.0-3272444.x86_64.bundle, so it's just a matter of mapping the amd64 from $::architecture to x86_64

@amosshapira
Copy link
Contributor Author

OK, with this latest commit I was able to successfully execute:

puppet apply -e 'class { "vmware_workstation": ensure=>"installed"}'

on Ubuntu 14.04.
Sorry about the noise - it was a bit tricky to test things on that specific Ubuntu box.

@mmarseglia mmarseglia self-assigned this Mar 10, 2016
mmarseglia added a commit that referenced this pull request Mar 10, 2016
Test $::architecture also against values used by Debian/Ubuntu.  This adds support for Debian/Ubuntu.
@mmarseglia mmarseglia merged commit 155750e into mmarseglia:master Mar 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants