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

Fixing issues when puppet runs as service #24

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Vizibirka
Copy link

If puppet runs as service the following exceptions are raised:
The server threw an exception. (Exception from HRESULT: 0x80010105
(RPC_E_SERVERFAULT))
You cannot call a method on a null-valued expression.

Surrunded the method with try-catch and added better error handling
logic

Added a "virtual" provider and .NET based method, which is available
from windows .NET 4.5 (Windows 8) and made it default.

But left the original as a fallback.

If puppet runs as service the following exceptions are raised:
The server threw an exception. (Exception from HRESULT: 0x80010105
(RPC_E_SERVERFAULT))
You cannot call a method on a null-valued expression.

Surrunded the method with try-catch and added better error handling
logic

Added a "virtual" provider and .NET based method, which is available
from windows .NET 4.5 (Windows 8) and made it default.

But left the original as a fallback.
) {
validate_absolute_path($destination)

if (! $creates and ! $refreshonly and ! $unless){
fail("Must set one of creates, refreshonly, or unless parameters.\n")
}

unless( $provider == 'dotnet' or $provider == 'com'){
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened to the old default of powershell? Should that be supported for backwards compatibility?

Also, some funky spacing in this line.

fail("Wrong provider: `${provider}', choices are: dotnet or com!\n")
}

if ($provider == 'dotnet'){
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra space


if ($provider == 'dotnet'){
$command_template = 'windows/unzip_dotnet.ps1.erb'
}else{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing space

@Vizibirka
Copy link
Author

Based on your observations, I fixed spacing plus extended the readme.

How do you mean by 'What happened to the old default of powershell? Should that be supported for backwards compatibility?'

Powershell is still there and does the job. I just extended the parameters' validation because if you run the following code against your original code it'll be green instead of failing.

windows::unzip { '\server.which.does\not\exists.zip':
destination => 'C:\dest',
creates => 'C:\dest\uncompressed.txt',
}

@jdavisp3
Copy link
Contributor

How do you mean by 'What happened to the old default of powershell? Should that be supported for backwards compatibility?'

Thinking about this some more, I have two points:

  1. Powershell used to be the default and now the default is dotnet, which is a change in behavior. I think it would be better to retain powershell as the default and allow dotnet as an option. At least it's safer for existing users for which everything is working fine now.
  2. The literal value of the provider argument that selected powershell used to be powershell and now it is com. Why not just keep it as powershell? I don't see the point of changing that, it just seems to introduce gratuitous differences between the old and the new version.

@Vizibirka
Copy link
Author

Vizibirka commented Jul 16, 2018

I see.

I thought about and I your're right, we should keep powershell as the provider and a new property should be added for fallbacking to your original implementation. I'll create another pull request

… deployments.

Modified readme to match with the ipmlementation.
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.

2 participants