Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Read Only Tenant Mode #4498
Read Only Tenant Mode #4498
Changes from 21 commits
b5fd84b
34b6105
ad6bac0
39a08e6
8e34af2
3f7799a
95ed066
83dfe9a
edffd9d
e71d72f
97ffecf
1bcf651
27ae5b5
b02687d
5b921d9
e55caae
b64a6af
e1b0d8e
4899570
77d3e69
79da52d
75b755a
66100c0
7f5ad08
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 amazing! Thanks for adding this readme. Makes the feature so much easier to use and understand.
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 license header is not correct I suppose.
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.
Yeah this is the right license header
The same goes for all the license headers in this PR
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 license header is not correct I suppose.
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 do you think of changing the output from being boolean to be an enum that has values like
ReadOnly, ReadWrite, Unknown
?I think this would better describe results for inline reading especially in the controllers where its hard to 'see' what the impact of a true vs false will be.
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.
@peternied then it wouldn't be a
ReadonlyService
butTenantCapabilityService
or something like that. As long as it has an impact only on capabilities I suggest keeping it as it is right now and extending that in the next PR :) Especially as long capabilities are only boolean and will not contain enum values.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.
Seems
isReadonly
method currently always returns false and thehideForReadonly
method will always return thecapabilites
argument as-is, without merging it with hideCapabilities. Seems to me that these functions are redundant or unnecessary. Is the current isReadonly method a placeholder, intended to be overridden or expanded upon in the future? should we add a comment on them?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.
@ananzh purpose is to keep working even without
security-plugin
(btw. here is an extension of the implementation https://github.com/opensearch-project/security-dashboards-plugin/pull/14720). For more context please follow opensearch-project/security#2701 (comment)