-
Notifications
You must be signed in to change notification settings - Fork 13
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
UI: 'Services' > 'Is managed by' #67
Conversation
This PR depends on this one to be merged. Until then, it is expected the tests to fail. |
The 'Services' > 'Is managed by' section should contain those entities that are allowed to manage the hosts of a selected service. That will be defined as part of the settings, alongside with the 'Is a member of' section. Signed-off-by: Carla Martinez <[email protected]>
bfda60a
to
179ac01
Compare
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 is working, it follows the same practices as in other parts of code. Thus approving.
But same comment I had as in the other PRs. We will need to enhance it in a future:
- a lot of stuff will need to be rewritten when we will start using real data and will need to work only with subset of data related to this context - i.e. also loading the right data at right time (when they are needed).
- adjust some of the other components like
ManagedByToolbar
orManagedByAddModal
to do a single concern.
i.e. something mentioned in the 2 comments.
const getFullHostInformation = (hostNameList: string[]) => { | ||
const fullHostList: Host[] = []; | ||
|
||
availableHostsData.map((host) => { |
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.
Future warning: when we will be playing with real data, you might find out that we don't always have all the Host which are mention in an association (e.g. this service's managed_by_host list).
Imagine that there is 10 000 host entries in the FreeIPA server and the service is managed by 1 host and some group of users. For this, we will probably not load all 10K of hosts to select the one which we want to display in the table.
So these places will (in the future) either need to load details of the related hosts, or we will need to be satisfied with the info returned in the service's managed_by_host attribute.
</PageSection> | ||
<> | ||
{showAddModal && ( | ||
<ManagedByAddModal |
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.
Interface of the ManagedByAddModal
is kinda hard to understand when we don't know the implementation.
E.g. I'd expect to have something like:
<AssociationAddModal
chosenEntries={managedByHosts}, // containing list of { key: host.uid, value: hostName }
availableEntriesLoader={functionToProvidePromiseOfListOfAvailableHosts} // promise would return same data structure as ^^
onSelectionChange={callback(selectedEntries)}
title="string"
/>
This would be usable for all the simple associations.
Thanks for the review and the feedback provided in the comments! I agree, some components' logic and structure can be improved and this can be done at some point in the future. Merging this PR. |
Description
The 'Is managed by' section provides a way to manage the entities by which a host from a specific Service can be managed. In this case, it refers to the hosts managed by other hosts.
Data type and initial data
Since the data type used (Host) was already defined in the
globalDataTypes.ts
file, there is no extra step to do here.Also, the initial data is taken from the service data that is passed to the
ServicesManagedBy
component via props.'Add' and 'Delete' modals
The 'Is managed by' section is using the existing components
ManagedByAddModal
andManagedByDeleteModal
for adding and deleting elements from a modal in the table:Table and Toolbar components
Similar to the modals, the 'Is managed by' is using the existing toolbar and table components (
ManagedByToolbar
andManagedByTable
respectively) to provide the functionality.Signed-off-by: Carla Martinez [email protected]