Skip to content
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

✨ Add server.getConfig endpoint to ozone #2494

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

foysalit
Copy link
Contributor

This helps ozone client to be aware of what the server is capable of and adjust the UI accordingly.

Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pumped for this! Dropped a few comments and questions.

Comment on lines 25 to 27
"viewerRole": {
"type": "string"
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can future-proof this by making this instead: viewer: { role }. That way if there is more viewer-specific info, as I expect there could be, we have a place to put it.

Comment on lines 10567 to 10575
properties: {
configured: {
type: 'boolean',
},
url: {
type: 'string',
format: 'uri',
},
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of configured? For example, would it ever make sense to serve { url, configured: false }?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A related thing to look at is whether any fields should be required. Right now all of these are in play, for example, and I think they may be interpreted the same way:

appview: undefined
appview: {}
appview: { configured: false }
appview: { configured: false, url }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can infer from the presence of url if the service is configured or not which makes the configured prop redundant.

Comment on lines +10571 to +10574
url: {
type: 'string',
format: 'uri',
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the ozone UI ever need to use this URL, or is its presence simply used to determine whether certain functionality appears in the UI?

One reason I ask is because another approach to this could be to serve flags that advertise what functionality the Ozone backend will support for a given user, and in turn what functionality the Ozone UI should display. This could take into account both the backend's configuration as well as the user's role.

I've had some luck with this in the past because the backend is the defacto source of truth for what functionality is available to the user, and the alternative is for both the backend and frontend to interpret config/roles/ACLs independently and come to the same conclusions about which features are available, which can be prone to error.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think I like this approach as well 👍

Although I still think it might be useful to return some of this config info just for giving additional details when navigating Ozone. For instance, just being able to see what your authz level is and maybe a "config/introspection" panel where you can see the services that Ozone is hooked up to

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as of now, this is just for display/introspection purposes. my thinking is that the backend will publish generic config and the frontend will determine, based on the raw config, which features to enable.
however, I'm open to BE publishing capabilities based on viewer role too. how does that look like in your mind? is it just an array of strings that contain the actions a user can perform, i.e: allowed: ['listUsers', 'listTemplates']? or would it be more evolved than that?

Copy link
Collaborator

@devinivy devinivy May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it could essentially look like a set of flags, which might be modeled as a list like that or like { listUsers: true, listTemplates: true }. I slightly prefer the latter, since it's more flexible if some of them call for a value other than a boolean.

It would have to be able to express more than which endpoints are available to the user, since there are some access rules like "can takedown account" which is a specific usage of the emitEvent endpoint. The sweet spot may be to make each flag a logical grouping of capabilities such as "can communicate with users" / "can view communication templates" / "can manage communication templates" / "can takedown accounts via label" / "can takedown accounts from infrastructure".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright we talked it out a bit ago but wanted to keep some traces here. as a next step, I will get a frontend PR up to better clarify how these configs will be interpreted and used on the frontend before making a final call on whether or not the backend should send more detailed and granular permission definition.

Comment on lines 45 to 47
"role": {
"type": "string"
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want to remember to sync this with your work in #2460 👍

Should we look ahead and at least start by using compatible role values with the ones used in that PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants