Allow creating roles for PostgreSQL#33
Conversation
| createRolesQuery = createRolesQuery.concat(";"); | ||
| } | ||
|
|
||
| if (((Map) role.getValue()).containsKey("privilages")) { |
There was a problem hiding this comment.
typo "privilages" -> "privileges"
also couple of lines below (could put this in a constant)
|
Thanks @geomacy |
| "Set roles with properties and permissions. Shoud be a map with keys equal to role names and values a map of the type:" + | ||
| "key equal to `properties` and value - the role properties that should be in the query after `WITH` statement" + | ||
| "key equal to `privileges` and value - the `GRANT` query value between the `GRANT` and `TO` statements" | ||
| ); |
There was a problem hiding this comment.
Maybe worth to add an actual example within the description of what the map should look like?
|
Thanks @geomacy @tbouron @duncangrant |
|
I was a little late getting to this, but it looks like @tbouron, @geomacy, and @duncangrant have already raised the concerns I had Still not completely convinced that this is required in addition to the database initialisation script. Any thoughts anyone? |
|
I agree, the user writing the queries in the yaml is the one who's deploying the entity and creating the database and can do whatever would like with them. But if this code is used for effectors in future or reused for anything, it's good to be secure from now. |
|
|
||
| private String buildCreateRolesQuery() { | ||
| Map<String, Map> roles = entity.getConfig(PostgreSqlNode.ROLES); | ||
| String createRolesQuery = "\""; |
There was a problem hiding this comment.
It is better to use StringBuilder.
ivanayov
force-pushed
the
postgresql-roles
branch
2 times, most recently
from
615dc3b to
844aa67
Compare
| String DEFAULT_USERNAME = "postgresqluser"; | ||
|
|
||
| @SetFromFlag("roles") | ||
| ConfigKey<Map<String, Map>> ROLES = new MapConfigKey(Map.class, "postgresql.roles", |
There was a problem hiding this comment.
Can we have a test for this?
ivanayov
force-pushed
the
postgresql-roles
branch
from
844aa67 to
fc808a4
Compare
|
@aledsage are you ok with merging this? |
|
@iyovcheva I've opened ivanayov#1 for some additional changes to the roles code. Can you take a look and see what you think please? If you merge that, it should appear automatically on this PR. |
ivanayov
force-pushed
the
postgresql-roles
branch
from
2a011e5 to
fec0b9d
Compare
|
@iyovcheva there's a merge conflict - can you rebase against master please? |
ivanayov
force-pushed
the
postgresql-roles
branch
from
fec0b9d to
2f24577
Compare
| location: localhost | ||
| services: | ||
| - type: org.apache.brooklyn.entity.webapp.ControlledDynamicWebAppCluster | ||
| - serviceType: org.apache.brooklyn.entity.webapp.ControlledDynamicWebAppCluster |
There was a problem hiding this comment.
Why change this? type should work as well. This file seems unrelated to the rest of the pull request.
There was a problem hiding this comment.
The test JavaWebAppsMatchingTest works for me locally. Can you explain these changes please?
9 participants
No description provided.