Pe client tools helper #15
Conversation
|
Can one of the admins verify this patch? |
|
Refer to this link for build results (access rights to CI server needed): |
|
|
||
| def self.path_separator(host) | ||
|
|
||
| (host.platform =~ /win/) ? '\\' : '/' |
There was a problem hiding this comment.
@er0ck If it is I don't know about it.
There was a problem hiding this comment.
sigh.... i'd hope that Command.cmd_line would take care of this, but alas it appears it does not.
This commit adds three helper methods to install pe-client-tools on Windows. The first is a general method that is designed to abstract away the installation of pe-client-tools on supported operating systems. Currently, it only accommodates development builds of the tools based on the provided SHA and SUITE_VERSION environment variables available. The second is a generic method to install an msi package on a target host. Beaker's built in method of this name assumes that msi installed involves the installation of puppet, so this method overrides that one without such an assumption. The this is a generic method to install a dmg package on a target host. Beaker's built in `install_package` method for osx does not accommodate for an installer `pkg` file that is named differently from the containing `dmg`. This method forces the user to supply both names explicitly.
|
@zreichert I have pushed an updated to my topic branch portion of this code in order to remove the msi and dmg installers (and change the calls to them). This branch is at https://github.com/johnduarte/beaker-pe/tree/jrd_pe_client_tools_helper The installer methods have been added to beaker proper. voxpupuli/beaker#1192 |
This commit removes the dmg and msi helper methods instroduced earlier. These two methods have bee moved into beaker.
zreichert
force-pushed
the
pe_client_tools_helper
branch
from
ef72651 to
358748b
Compare
|
@johnduarte I have added that branch to this PR. Take a look to see if this looks correct. We should probably squash your two commits and rewrite the commit message. |
|
Refer to this link for build results (access rights to CI server needed): |
|
@zreichert I have validated that this PR works properly with puppet-code for both OSX and Windows pe-client-tools packages. |
| if host.platform =~ /win/i | ||
|
|
||
| program_files = host.exec(Beaker::Command.new('echo', ['%PROGRAMFILES%'], :cmdexe => true)).stdout.chomp | ||
| client_tools_dir = "#{program_files}\\#{['Puppet Labs', 'Client', 'tools', 'bin'].join('\\')}\\" |
There was a problem hiding this comment.
doesn't PEClientTools::ConfigFileHelper figure this out, with some pain, above?
|
@kevpl this is blocking work on pe-client-tools validation. What changes are necessary in order to get this merged? cc @zreichert |
| else | ||
| raise "install_puppet_agent_on() called for unsupported " + | ||
| "platform '#{host['platform']}' on '#{host.name}'" | ||
| end |
There was a problem hiding this comment.
I would suggest refactoring this section that's really looking to get host-specific install information into a helper method. install_puppet_agent_pe_promoted_repo_on has been refactored in this way in lines 1171-81, and I think it's made the method much more understandable and maintainable.
It's not as good that you won't be able to make it a host method at this time, but a helper in this same file containing these host-specific details would still improve this method quite a bit, and has the advantage that having it here rather than a host method is closer to you & your code, and as such future changes will be easier to make than in beaker-core.
To combat the issue that there's an additional installer variable needed for osx, that variable is only used once, and it's in a line that's already mac-specific, so I would suggest just refactoring the osx install line to use
"#{package_name}-installer.pkg"as the parameter instead of needing the installer variable.
|
Per the conversation I just had with @zreichert & @johnduarte, the only things that are completely halting merging this are having some testing of this functionality. Since QA is under time pressure, I've agreed to have these refactorings ticketed as future tech-debt work. @er0ck, I couldn't really speak to your comments. Any feedback on this? |
|
if it were me, i'd just commit the libs locally to my project even if repetitive, and then refactor the code for commit to beaker-pe. but i wouldn't blame you if you merged this as-is either. |
zreichert
force-pushed
the
pe_client_tools_helper
branch
from
80aea0c to
97b25ea
Compare
zreichert
force-pushed
the
pe_client_tools_helper
branch
from
97b25ea to
f1f0a46
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Build finished. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| when /windows/ | ||
| release_path = "#{opts[:dev_builds_url]}/#{product}/#{ opts[:pe_client_tools_sha] }/artifacts/#{variant}/#{opts[:puppet_collection]}/x#{arch}" | ||
| package_name = product.dup | ||
| package_name << "-#{opts[:pe_client_tools_version]}.1-x#{arch}_VANAGON.msi" if opts[:pe_client_tools_version] |
There was a problem hiding this comment.
This will need to change when https://github.com/puppetlabs/pe-client-tools-vanagon/pull/88 gets merged, because it will remove the VANAGON suffix from the filename.
|
@kevpl these acceptance failure look legit, but completely unrelated to this PR. Thoughts? |
|
@puppetlabs-jenkins retest this please |
|
Refer to this link for build results (access rights to CI server needed): |
|
@kevpl 🍏 |
|
created BKR-898 to handle the refactorings that we discussed here at a later time. |
No description provided.