-
Notifications
You must be signed in to change notification settings - Fork 21
(CONT-567) allow deferred function for password #436
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
Conversation
Ramesh7
commented
May 18, 2023
•
edited
Loading
edited
- implementing deferred function for password field
sqlserver::password is a functionthat may have no external impact to Forge modules. sqlserver::login is a typethat may have no external impact to Forge modules. sqlserver::user is a typethat may have no external impact to Forge modules. This module is declared in 3 of 580 indexed public
|
54037ca
to
7cf4081
Compare
end | ||
|
||
# the function below is called by puppet and and must match | ||
# the name of the puppet function above. You can set your |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfinished comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!!
# @summary This function exists for usage of a role password that is a deferred function | ||
Puppet::Functions.create_function(:'sqlserver::password') do | ||
dispatch :password do | ||
optional_param 'Any', :pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interested in the types here.
I wonder if we should be more prescriptive? For example, would String
be a better choice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chelnak the password is not mandetory field here so restricting with String will lead to make it mandetory. That's why I have added to any type.
Ref : https://github.com/puppetlabs/puppetlabs-sqlserver/blob/main/manifests/login.pp#L59
Please let me know if we wanted to make it required field.
manifests/user.pp
Outdated
sqlserver_tsql { "user-${instance}-${database}-${user}": | ||
instance => $instance, | ||
command => template("sqlserver/${create_delete}/user.sql.erb"), | ||
command => Deferred('inline_epp', [file("sqlserver/${create_delete}/user.sql.epp"), $parameters]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@binford2k added a deferrable_epp function to stdlib.
Could we use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added Deferred
looking at compatibility. The deferrable_epp
recently got added and will not available with Puppet 7.
If we make sure it will always get used with Puppet 8 then we are good to deferrable_epp.pp
otherwise we still need to stick with Deferred
.
Please suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I haven't see any references on doc about https://www.puppet.com/docs/puppet/8/deferring_functions.html so stick with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chelnak addressed your comment, can you please re-look. Thanks
1cea6e2
to
a13e824
Compare