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

(maint) Properly install pe-client-tools when using a tag version #69

Conversation

nicklewis
Copy link
Contributor

Previously, installing pe-client-tools with a tag version would fail on
Windows/OS X and install the wrong package on Linux.

When installing pe-client-tools, we provide two options:

  • pe_client_tools_sha: the commit SHA of the version to install
  • pe_client_tools_version: the git describe of the version to install

pe_client_tools_version is always the name of the package to install.
But the location of the package differs based on whether the package
version corresponds to a tag or not. When the package isn't a tag
version, it's located in a directory named based on the SHA. But when it
is a tag version, it's located in a directory named after the tag.

When pe_client_tools_version was specified as a tag, we would look in
the directory named after the SHA (which was actually from a previous
build of the package, from before it was tagged) for a file named after
the tag. That file would never be there, since we had a mismatch of
directory and filename. For Windows and OS X, this caused a failure to
install, because they need to know the exact filename.

This case incidentally worked (or appeared to work) on Linux
platforms, because they never actually refer to the package by
filename. Instead, they install the package by setting up a repo config,
which is always named after pe_client_tools_sha, and never
pe_client_tools_version. In that case, the Linux platforms would
actually install the previous version of the package by SHA, from before
it had been tagged.

We now properly handle the case where pe_client_tools_version is a tag,
by using that version as the location of the file in addition to the
filename.

Previously, installing pe-client-tools with a tag version would fail on
Windows/OS X and install the wrong package on Linux.

When installing pe-client-tools, we provide two options:
- pe_client_tools_sha: the commit SHA of the version to install
- pe_client_tools_version: the `git describe` of the version to install

pe_client_tools_version is always the name of the package to install.
But the *location* of the package differs based on whether the package
version corresponds to a tag or not. When the package isn't a tag
version, it's located in a directory named based on the SHA. But when it
is a tag version, it's located in a directory named after the tag.

When pe_client_tools_version was specified as a tag, we would look in
the directory named after the SHA (which was actually from a *previous*
build of the package, from before it was tagged) for a file named after
the tag. That file would never be there, since we had a mismatch of
directory and filename. For Windows and OS X, this caused a failure to
install, because they need to know the exact filename.

This case incidentally *worked* (or appeared to work) on Linux
platforms, because they never actually refer to the package by
filename. Instead, they install the package by setting up a repo config,
which *is* always named after pe_client_tools_sha, and never
pe_client_tools_version. In that case, the Linux platforms would
actually install the previous version of the package by SHA, from before
it had been tagged.

We now properly handle the case where pe_client_tools_version is a tag,
by using that version as the location of the file in addition to the
filename.
@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

@puppetlabs-jenkins
Copy link
Contributor

Refer to this link for build results (access rights to CI server needed):
http://jenkins-beaker.delivery.puppetlabs.net//job/qe_beaker-pe_btc-intn/201/

@nicklewis
Copy link
Contributor Author

@puppetlabs/beaker If anyone has a moment, this issue is breaking orchestrator-client tests, so I'd love a review.

Copy link
Contributor

@kevpl kevpl left a comment

Choose a reason for hiding this comment

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

@puppetlabs/integration any reason we shouldn't release this today for the weekend jobs? Or does this not impact those?

@jpartlow
Copy link
Contributor

jpartlow commented May 5, 2017

@kevpl This looks fine to merge and release. I don't believe we install the client tools separately in any of the integration pipeline tests (pe_acceptance_tests).

@nicklewis
Copy link
Contributor Author

Seems like this is ready to go. Does it need any additional input from me?

@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

@MikaelSmith
Copy link
Contributor

ping

@jpartlow
Copy link
Contributor

There is an unreleased commit from #66 that I want to check with @cthorn42 about; but I suspect I can merge this and release a new beaker-pe in the morning.

@jpartlow
Copy link
Contributor

Since pe_acceptance_tests is pinned to 1.11.0 on all branches, I'm going to go ahead and merge and release this change.

@jpartlow jpartlow merged commit e162d8e into puppetlabs:master May 10, 2017
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.

5 participants