-
Notifications
You must be signed in to change notification settings - Fork 80
Images refresh batch 2 #2541
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
base: dev
Are you sure you want to change the base?
Images refresh batch 2 #2541
Conversation
…neo4j#2490) Since we moved them over from DBMS level (while still keeping the DBMS level syntax, just as another syntax for `DATABASE *`) --------- Co-authored-by: Mark Dixon <1756429+mnd999@users.noreply.github.com> Co-authored-by: Reneta Popova <reneta.popova@neo4j.com>
The Operations manual covers all releases of 2025.xx series. That's why we cannot simply replace examples with new ones, as new functionality has been added. We also need to retain examples for earlier versions of Neo4j.
On the page Database privileges, Figure 2. Syntax of GRANT and DENY database privileges is not updated. |
On the page DBMS privileges, the Figure 1. Syntax of GRANT and DENY DBMS privileges is not updated. |
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 think, the branch (on the right hand side) for SERVER MANAGEMENT -> SHOW SERVERS, EXECUTE Privileges, and SHOW SETTINGS privilege is unclear and might be wrong.
There are three equal groups:
- SERVER MANAGEMENT (with SHOW SERVERS at the 2nd level),
- EXECUTE PRIVILEGES (that include EXECUTE PROCEDURES, EXECUTE BOOSTED PROCEDURES, EXECUTE ADMIN(ISTRATOR) PROCEDURES, EXECUTE FUNCTIONS, and EXECUTE BOOSTED FUNCTIONS),
- and SHOW SETTINGS.
I think, we should have three equal branches for them. At the moment, their hierarchy is unclear.
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.
The EXECUTE privileges aren't a group in the hierarchy: they don't have a common parent privilege.
They are included under ALL DBMS PRIVILEGES
as that one includes EXECUTE FUNCTION, EXECUTE BOOSTED FUNCTION, EXECUTE PROCEDURE and EXECUTE BOOSTED PROCEDURE for all functions and procedures (which therefore also covers the EXECUTE ADMIN PROCEDURES as that one includes EXECUTE PROCEDURE and EXECUTE BOOSTED PROCEDURE but only for procedures marked as admin procedures). So the old image that currently exist in the documentation is correct to have them just on a line and not in a group, as the 'groups' are actual privileges themselves.
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 if to make frames of the first level blocks bold? These blocks are:
- ROLE MANAGEMENT
- USER MANAGEMENT
- IMPERSONATE
- SERVER MANAGEMENT
- DATABASE MANAGEMENT
- ALIAS MANAGEMENT
- PRIVILEGE MANAGEMENT
- EXECUTE PRIVILEGES (I think they could be grouped together)
- SHOW SETTINGS
@@ -76,7 +76,7 @@ For more details on the differences between graphs, databases, and the DBMS, ref | |||
|
|||
image::privileges_grant_and_deny_syntax_dbms_privileges.svg[width="800", title="Syntax of GRANT and DENY DBMS privileges"] |
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.
image::privileges-grant-and-deny-syntax-dbms-privileges.svg[width="800", title="Syntax of GRANT and DENY DBMS privileges"]
@@ -480,7 +480,7 @@ GRANT [IMMUTABLE] TRANSACTION [MANAGEMENT] [( { * \| user[, ...] } )] | |||
|=== | |||
|
|||
|
|||
image::privileges_grant_and_deny_syntax_database_privileges.svg[title="Syntax of GRANT and DENY Database Privileges"] | |||
image::privileges-grant-and-deny-syntax-database-privileges.svg[title="Syntax of GRANT and DENY Database Privileges", role=popup] |
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 don't understand what is happening here. In line 187, image::privileges_hierarchy_database.svg[title="Database privileges hierarchy"]
is used instead of the new file name with hyphens. However, the new image is rendered in the preview.
The file name is updated in the line 483, but the old image is rendered.
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 feel like the blocks are hard to read and the hierarchy structure is harder to see with this layout :(
(I assume this applies to more than just this one of the hierarchy pictures but I only opened this one so far)
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.
it looks better in the preview than here on github though 🤷
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 doesn't look like the latest version of this image, the development page version (https://development.neo4j.dev/docs/operations-manual/current/authentication-authorization/database-administration/) has some updates (regarding the four ALTER DATABASE privileges) that isn't reflected here
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.
(nor are they in the hierarchy version)
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.
Thank you, @lidiazuin. They all look good from a visual perspective but I've found some inconsistencies and things that might lead to misunderstandings.
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.
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.
#2541 (comment) I also noticed this :P
modules/ROOT/pages/authentication-authorization/database-administration.adoc
Outdated
Show resolved
Hide resolved
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.
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.
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.
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.
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.
The connections that you noted that don't derive from the same place are all coming from ALL DBMS PRIVILEGES, that's the style of the arrows used in the technical diagrams. I can try to change the style, but just letting you know that this is the default styling.
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 thought so, but they look misleading, so I decided to tell you if you can change them somehow.
modules/ROOT/pages/authentication-authorization/load-privileges.adoc
Outdated
Show resolved
Hide resolved
modules/ROOT/pages/authentication-authorization/dbms-administration.adoc
Outdated
Show resolved
Hide resolved
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.
There is no space between To role
and [,...]
in the original diagram. @Hunterness, is the space a problem?
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 don't think it matters as such but generally I'd not have a space there if I write the command even if it parses (... TO role1, role2
vs ... TO role1 , role2
) 🤷
I'll also say that if the other syntax images doesn't have the space I don't think we should have it here but be consistent about it
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.
Co-authored-by: Reneta Popova <reneta.popova@neo4j.com>
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.
Review of current 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.
Missing the various ALTER DATABASE privileges (ALTER DATABASE, SET DATABASE ACCESS, SET DATABASE DEFAULT LANGUAGE, ALTER COMPOSITE DATABASE), and the *|name[, ...]
after DATABASE[S]
should not have any ()
as that isn't part of the syntax for the database part as it is for the user part after transactions.
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.
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.
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.
The EXECUTE privileges are still not a group: the groups are existing privileges that also cover the sub-privileges and there isn't an EXECUTE privilege so it isn't a group
These hierarchy images are supposed to show the relationships between privileges and not just another way to show what privileges exist, so adding groupings that doesn't correspond to existing privileges are not valid
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.
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.
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.
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 goes for all syntax images and not just load.
In our old pictures we marked keywords and input values differently, the keywords were in capital letters and the input was in italics and small letters, you still have the capital vs small letter diff but are missing the italics which helped separate them further
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.
Unfortunately the font used now doesn't have an italic version... Is there another way we could point to that you think would be useful? For TO role[,...]
, for example, the bubble is white
@@ -210,7 +210,7 @@ image::privileges_on_graph_syntax.svg[width="800", title="Syntax of GRANT and DE | |||
|
|||
The following image shows the hierarchy between different graph privileges: | |||
|
|||
image::privileges_hierarchy.svg[title="Graph privileges hierarchy"] | |||
image::privileges-hierarchy.svg[title="Graph privileges hierarchy", role=popup] |
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.
so you update this one but not the two above?
image::privileges_grant_and_deny_syntax.svg[width="800", title="GRANT and DENY Syntax"]
image::privileges_on_graph_syntax.svg[width="800", title="Syntax of GRANT and DENY Graph Privileges. The `{` and `}` are part of the syntax and not used for grouping."]
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 like that this one have arrows, I'd like the other hierarchy ones to also have that
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.
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, to me having SET PROPERTY be in the middle layer feels odd, hierarchy-wise it's on the same level as for example CREATE (two steps away from the top ALL [GRAPH [PRIVILEGES]]
one) so putting it on the level with the MATCH/MERGE/WRITE that is one step from the top feels odd.
But it does avoid having to have lines crossing each other 🤷
@@ -27,5 +27,4 @@ Then, the Neo4j Helm chart creates Kubernetes entities needed for running and ac | |||
The following diagram is a schematic representation of the Helm chart involved and the Kubernetes and Cloud resources it instantiates when installed: | |||
|
|||
.Neo4j standalone setup | |||
image::standalone-on-k8s.svg[Example of Neo4j standalone setup in Kubernetes,width=700,role=popup] | |||
|
|||
image::standalone-on-k8s.svg[Example of Neo4j standalone setup in Kubernetes,width=700,role=popup] |
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 PR includes documentation updates Updated pages: |
No description provided.