-
Notifications
You must be signed in to change notification settings - Fork 64
Add SHOW SETTING clause and privileges #440
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
|
Looks like you've updated the documentation! Check out your changes at https://neo4j-docs-cypher-440.surge.sh |
Hunterness
left a 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.
I have not reviewed the main documentation for the command yet but you can get some comments in the meantime (as I'll continue tomorrow with the review)
modules/ROOT/pages/deprecations-additions-removals-compatibility.adoc
Outdated
Show resolved
Hide resolved
modules/ROOT/pages/deprecations-additions-removals-compatibility.adoc
Outdated
Show resolved
Hide resolved
l-heemann
left a 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.
Minor suggestions
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.
Some missed things:
- Execution plan is missing (the
operators.adocandoperator-summary.adocfiles) - Not sure if the built-in roles section have any lists of capabilities that needs updating, but it should be checked and if yes, updated
- The DBMS privilege images (hierarchy and general syntax outline) needs to be updated (the original currently live in google docs and are then saved as svg files that we update in the docs repo)
modules/ROOT/pages/deprecations-additions-removals-compatibility.adoc
Outdated
Show resolved
Hide resolved
modules/ROOT/pages/deprecations-additions-removals-compatibility.adoc
Outdated
Show resolved
Hide resolved
Co-authored-by: Therese Magnusson <scout.therese@gmail.com>
no changes to built-in roles as we have implicitly included the |
l-heemann
left a 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.
Consider if we want to re-use the name-globbing node in privileges_grand_and_deny_syntax_dbms_privileges.
Annoying that the edges now overlap as well, but I couldn't get them to behave either so...
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.
Comments on the images:
-
privileges_hierarchy_dbms.svg: The SHOW SETTINGS should not have the*. Yes that is technically which version is included but we don't have that level detail in the image (compare withEXECUTE) -
privileges_grant_and_deny_syntax_dbms_privileges.svg: I agree with Lasse's reuse of globbing box comment. I'm also annoyed at the crossing lines so I might take a stab at them as well
Edit: I've updated the images in the drive folder for both of these, now they just need to be updated on the PR as well
|
Question for the docs team: Is there any reason to have the png images n the repo? as far as I'm aware we're using the svg versions in the text? (I also though the png ones had been removed when the svg ones were introduced?) |
Co-authored-by: Therese Magnusson <scout.therese@gmail.com>
Hunterness
left a 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.
Looks good to me 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.
Great work, @ali-ince !
I have a few suggestions and comments. Also, I was thinking maybe we should include a note on the clauses page explaining that this replaces dbms.listConfig and that, unlike dbms.listConfig, this is something Aura users can use?
modules/ROOT/pages/deprecations-additions-removals-compatibility.adoc
Outdated
Show resolved
Hide resolved
Co-authored-by: Jens Pryce-Åklundh <112686610+JPryce-Aklundh@users.noreply.github.com>
Co-authored-by: Jens Pryce-Åklundh <112686610+JPryce-Aklundh@users.noreply.github.com>
We initially had a similar note, but then decided to remove it since we don't actually remove or deprecate |
|
@ali-ince @Hunterness - sorry I did not see your comment re .png images, and no, we should not keep the png files in the repo, but use .svg files instead. |
JPryce-Aklundh
left a 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.
Thanks, @ali-ince!
I missed this in my first review, but would it be possible to change the images to .svg?
With regard to Aura, I may add something to this page at a later date then explaining that, while dbms.listConfig is not deprecated/removed, SHOW SETTINGS is something Aura users can use.
I was wondering since Ali updated existing png files, I guess they have been accidentally moved over in some cherry-pick 🤷 (we have to be careful with those as we add new things to the images when introducing new rbac features and don't want those to be cherry-picked back nor removed in a forward cherry-pick)
They are, for some reason we have both in the repo (see above point) but I'm pretty sure it's the svg ones that are used in the includes |
|
I just checked, and you are correct, @Hunterness - it is the .svg images that are used in the includes. @ali-ince, please disregard my previous comment about changing the image formats. |
Might be a good idea to remove the png files as they are unused 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.
the original sentence here was very explicit that the examples might only use procedures but the rules also applied to functions and settings but I feel like it's less explicit now 🤔 I haven't found a good way to make it better 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 was a tricky one, @Hunterness, @ali-ince
How about this (together with my suggestions for lines 2121-2122)
| Given the list of procedures named as follows, the consecutive examples show valid name-globbing patterns that can be applied to them. | |
| The same rules apply to user-defined functions and settings. | |
| Given the following list of procedures: |
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.
| * `your.exampleProcedure` | |
| * `your.exampleProcedure` | |
| The following examples demonstrate how procedures with valid name-globbing patterns can be used in queries. | |
| Note that the same rules apply to user-defined functions and settings. |
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 switching them? Have this first, then the comment on given these procedures (and then maybe an additional comment to clarify where the examples start?) As the procedure list is more set-up to the examples it feels like we should have clarified why we only look at procedures before 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.
I think I find Jens' approach a bit more clear, but suggest we slightly change the last block;
The following examples demonstrate how name-globbing patterns can be used in queries.
Note that the same rules apply to user-defined functions and settings.
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 see any difference between your block and his?
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.
sorry, copy-paste error. fixed it 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.
I still want to mention something about the examples only using procedures?
|
@Hunterness @JPryce-Aklundh I've pushed another version for the remaining point that mentions procedures. If you both are happy with it, I'd like to merge these group of PRs for SHOW SETTINGS. |
|
It looks good to me 👍 |
This PR adds information about SHOW SETTING privileges.