-
Notifications
You must be signed in to change notification settings - Fork 21
FM-1556 Add ability to manage login server level permissions #72
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
Thought there was a bug, was not, it was an issue with test login having svrroles that caused failure. |
|
||
## Validate state | ||
$_state = upcase($state) | ||
validate_re($_state,'^(GRANT|REVOKE|DENY)$', "State can only be of 'GRANT', 'REVOKE' or 'DENY' you passed ${state}") |
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.
Nit - bad grammar
Added a note to the ticket about getting a capture of successful manifest runs against SQL Server since there are no acceptance tests. |
# The login for which the permission will be manage. | ||
# | ||
# [permission] | ||
# The permission you would like managed. i.e. 'SELECT', 'INSERT', 'UPDATE', 'DELETE' |
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.
What about EXECUTE? Nevermind, these are examples.
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.
Please update documentation - this is permissions now.
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.
This is still not updated.
Nothing really jumps out at me as inherently bad here. Just small tweaks and of course the CASCADE thing. |
Requires #74 for tests to pass |
@ferventcoder @Iristyle Here is the latest, I modified last night to be able to pass an array of permissions instead of individual ones. This applies to both Login and User permissions. |
SET @permission = '<%= permission %>' | ||
<%= scope.function_template(['sqlserver/snippets/login/permission/exists.sql.erb']) %> | ||
<% end %> | ||
END |
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.
Same comment from #71 about the double loop.
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 to single loop
Same deal here with documentation. Get that updated and this is |
@ferventcoder docs updated |
No description provided.