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.

Sign up for GitHub

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

Jump to bottom

Issue 212 - implement hosts Add & Delete buttons #235

Merged
merged 1 commit into from
Jan 19, 2024
Merged

Issue 212 - implement hosts Add & Delete buttons #235

merged 1 commit into from
Jan 19, 2024

Conversation

mreynolds389
Copy link
Collaborator

fixes: #212

@mreynolds389 mreynolds389 requested a review from carma12 January 4, 2024 20:42
@mreynolds389 mreynolds389 added the WIP Work in Progress (do not merge) label Jan 4, 2024
@mreynolds389
Copy link
Collaborator Author

mreynolds389 commented Jan 4, 2024 •
edited
Loading

@carma12 - Not sure how DNS zones are supposed to work. I added code to retrieve all the zones but I'm not actually doing anything with them at the moment.

The old UI still wants the FQDN for the hostname, so why have a drop list of all the available dns zones? Because of this I am labeling this PR as WIP.

@mreynolds389 mreynolds389 force-pushed the add_host branch 3 times, most recently from 83f9bd2 to 737e7f0 Compare January 4, 2024 21:34
@mreynolds389 mreynolds389 self-assigned this Jan 4, 2024
@mreynolds389 mreynolds389 force-pushed the add_host branch 2 times, most recently from 5fd6cd4 to 45ad0e9 Compare January 8, 2024 15:39
@pvoborni
Copy link
Member

I can help answering some Qs form old UI. When adding a new host, it should be possible to add a host with arbitrary FQDN. Old UI tries to help the user by providing a list of DNS Zones in IPA so that the user might only fill the shortname part of FQDN. But, to be able to fill in arbitrary FQDN, there still needs to be a way to specify arbitrary "domain"/"DNS zone" for the host.

The behavior also changes when IPA is installed without DNS server. In such case, it doesn't manage DNS Zones and thus old UI offers (I think) only single textbox where user can provided the FQDN.

@mreynolds389
Copy link
Collaborator Author

@carma12 - could you start the review on this PR? The work I'm doing on the host settings is starting to overlap with this PR. Thanks!

carma12
carma12 requested changes Jan 16, 2024
View reviewed changes
Copy link
Collaborator

@carma12 carma12 left a comment

Choose a reason for hiding this comment

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

Please see the inline comments below.

src/components/modals/AddHost.tsx Show resolved Hide resolved
src/components/BulkSelectorUsersPrep.tsx Show resolved Hide resolved
src/components/modals/AddHost.tsx
@@ -46,12 +67,27 @@ const AddHost = (props: PropsToAddHost) => {
const hostIpAddressHandler = (value: string) => {
setHostIpAddress(value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to validate this field input to check if the inserted IP address format is valid (similar to what's done in the current WebUI). This can be done in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We might want to validate this field input to check if the inserted IP address format is valid (similar to what's done in the current WebUI). This can be done in a separate PR.

I'll look into this, or file a new issue...

src/components/modals/AddHost.tsx Outdated
// Add host data
const addHostData = async () => {
const newHostPayload = {
hostname: hostName,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be hostName + dnsZoneSelected. Check the image with an API call example below:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well the DNS zone stuff is what is in WIP. @pvoborni gave me additional info on that. I'll be working on this...

Copy link
Collaborator

@carma12 carma12 Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh ok, thanks for letting me know that. In any case, a friendly reminder that you still need to send the hostName + dnsZoneSelected to the payload (otherwise, it will throw an error). The current code only takes the hostName. See in the example below that only the host1 name (hostName) is being taken:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In any case, a friendly reminder that you still need to send the hostName + dnsZoneSelected to the payload (otherwise, it will throw an error). The current code only takes the hostName.

Sorry above I said that was still a WIP. I wasn't sure at the time how dns zone was used. So right now hostname expects FQDN, but I guess the old UI wanted a hostname that would be added to the DNS zone: server + ipa.demo ==> server.ipa.demo

src/components/modals/AddHost.tsx Show resolved Hide resolved
@mreynolds389 mreynolds389 force-pushed the add_host branch 3 times, most recently from 72eb749 to bd14e1e Compare January 17, 2024 14:48
@mreynolds389
Copy link
Collaborator Author

@carma12 - ok I think I fixed all the issues, please review...

@mreynolds389 mreynolds389 force-pushed the add_host branch 3 times, most recently from 45decc7 to 40df36d Compare January 17, 2024 16:41
carma12
carma12 requested changes Jan 18, 2024
View reviewed changes
Copy link
Collaborator

@carma12 carma12 left a comment

Choose a reason for hiding this comment

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

Both 'Add' and 'Delete' modals work as expected in terms of API calls. Although I detected some tiny issues to fix in the code.

src/components/modals/DeleteHosts.tsx Outdated
if ("data" in response) {
const data = response.data as BatchRPCResponse;
const result = data.result;
const error = data.error as FetchBaseQueryError | SerializedError;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This variable is not being used and can be safely removed.

src/components/modals/AddHost.tsx
}
} else {
// Clean data and close modal
cleanAndCloseModal();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This modal is resetting all the fields except the Description one, so it would be convenient to add its default value to the cleanAllFields function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This modal is resetting all the fields except the Description one, so it would be convenient to add its default value to the cleanAllFields function.

Nice catch, fixed. The unused variable is also now fixed. Please review...

@carma12 carma12 self-requested a review January 19, 2024 08:08
carma12
carma12 approved these changes Jan 19, 2024
View reviewed changes
Copy link
Collaborator

@carma12 carma12 left a comment

Choose a reason for hiding this comment

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

The changes LGTM, hence approving.

@mreynolds389
Issue 212 - implement hosts Add & Delete buttons
1e9602c 
fixes: freeipa#212

Signed-off-by: Mark Reynolds <mreynolds@redhat.com>
@mreynolds389 mreynolds389 removed the WIP Work in Progress (do not merge) label Jan 19, 2024
@mreynolds389 mreynolds389 merged commit d8527ee into freeipa:main Jan 19, 2024
2 checks passed
@mreynolds389 mreynolds389 deleted the add_host branch January 19, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carma12 carma12 carma12 approved these changes

Assignees

@mreynolds389 mreynolds389

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement "Add" host modal functionality
3 participants
@mreynolds389 @pvoborni @carma12