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

New argument to set exportability of the certificate #46

Merged
merged 2 commits into from Apr 30, 2017

Conversation

ricardogaspar2
Copy link
Contributor

@ricardogaspar2 ricardogaspar2 commented Mar 29, 2017

Adding a new argument to specify the location of the scripts. README updated with this info. Spec tests added. Tests use a different certificate thumbprint.The test certificate is spec/testCert.pfx

This is a combined pull it contains two features:
#45 (new argument to specify a directory to store the scripts) and #46 itself (flag to set the exportability of the certificate key).

…updated with this info. Spec tests added. Tests use a different certificate thumbprint.The test certificate is spec/testCert.pfx

fixing style errors,rubocop errors

 Committer: ricardogaspar2 <ricardogaspar2@gmail.com>
@ricardogaspar2
Copy link
Contributor Author

ricardogaspar2 commented Apr 19, 2017

Hi, can someone review these changes? @dalen , @apenney , @oranenj , @jfryman , @carlossg , @juniorsysadmin, @stack72 ,

ensure => directory
}
)

Copy link

Choose a reason for hiding this comment

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

I think the closing parenthesis should follow the brace on the same line.

$root_store = 'LocalMachine',
$store_dir = 'My',
$scripts_dir = 'C:\temp',
$is_exportable = true) {
Copy link

Choose a reason for hiding this comment

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

I'd prefer this to be just exportable instead.

Is 'My' a good default for store_dir?

This should probably be converted to use Puppet 4 datatypes, so that you can get rid of the validate calls below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, 'My' is a default dir. I just have extended the code, as you can see, I maintained the default options/parameters.

$key_storage_flags = 'Exportable,PersistKeySet'
}else{
$key_storage_flags = 'PersistKeySet'
}

Copy link

Choose a reason for hiding this comment

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

the parentheses around $is_exportable are superfluous, and you should have whitespace around else, I think

@ricardogaspar2
Copy link
Contributor Author

@oranenj please review the code after the changes you requested.
Thanks in advance

@oranenj
Copy link

oranenj commented Apr 27, 2017

This seems fine to me on a general level. Someone else will have to merge though, as I can't say I'm very familiar with the domain specifically.

@oranenj oranenj added the enhancement New feature or request label Apr 27, 2017
@ricardogaspar2
Copy link
Contributor Author

Do you know who can merge and when can it be merged?

@juniorsysadmin
Copy link
Contributor

Would have preferred two separate PRs, but merging anyway

@juniorsysadmin juniorsysadmin merged commit a3f40e4 into puppetlabs:master Apr 30, 2017
@Ramesh7 Ramesh7 added the feature New Feature label Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants