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

Hostname handling #167

Merged
merged 3 commits into from
May 8, 2024
Merged

Conversation

adamkankovsky
Copy link
Contributor

@adamkankovsky adamkankovsky commented Feb 8, 2024

image
image

src/components/review/ReviewConfiguration.jsx Outdated Show resolved Hide resolved
src/components/review/ReviewConfiguration.jsx Outdated Show resolved Hide resolved
src/components/review/ReviewConfiguration.jsx Outdated Show resolved Hide resolved
@adamkankovsky adamkankovsky force-pushed the hostname-handling branch 14 times, most recently from f1dabf9 to 10f1b21 Compare February 27, 2024 14:44
@adamkankovsky adamkankovsky force-pushed the hostname-handling branch 3 times, most recently from 3cb869f to ce9961c Compare April 16, 2024 10:44
@adamkankovsky
Copy link
Contributor Author

Updated to new design

@rvykydal
Copy link
Contributor

rvykydal commented Apr 23, 2024

I'd hate to slow down this PR but I'd consider using "network information" instead of "DHCP" as if I understand it correctly it can be set also just from DNS lookup, independently of DHCP (https://www.freedesktop.org/wiki/Software/systemd/hostnamed/ )?

Other than that I am fine with the design for this PR, thank you.

@rvykydal
Copy link
Contributor

rvykydal commented Apr 23, 2024

If I change a value from empty/transient to something, then I am not able to change it back to empty/transient. I think we should allow this (and add to tests if so).

Copy link
Contributor

@rvykydal rvykydal left a 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 with minor issues I would not block on, but I think we should allow to reset to transient.

src/apis/network.js Outdated Show resolved Hide resolved
}

if (!value.match(/^(?!-)[a-z0-9-]{1,63}(?<!-)$/)) {
validationError.push(_("Real hostname can only contain lower-case characters, digits, dashes (not starting or ending a label)"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't understand "not starting or ending a label"

src/components/review/ReviewConfiguration.jsx Outdated Show resolved Hide resolved
test/check-review Show resolved Hide resolved
@adamkankovsky
Copy link
Contributor Author

If I change a value from empty/transient to something, then I am not able to change it back to empty/transient. I think we should allow this (and add to tests if so).

Unfortunately there is a bit of a problem here, because Anaconda doesn't allow you to set an empty hostname, so it won't be possible to fix this without modifying the backend.

@rvykydal
Copy link
Contributor

I'd hate to slow down this PR but I'd consider using "network information" instead of "DHCP" as if I understand it correctly it can be set also just from DNS lookup, independently of DHCP (https://www.freedesktop.org/wiki/Software/systemd/hostnamed/ )?

Other than that I am fine with the design for this PR, thank you.

@garrett what do you think?

@rvykydal
Copy link
Contributor

rvykydal commented Apr 24, 2024

If I change a value from empty/transient to something, then I am not able to change it back to empty/transient. I think we should allow this (and add to tests if so).

Unfortunately there is a bit of a problem here, because Anaconda doesn't allow you to set an empty hostname, so it won't be possible to fix this without modifying the backend.

Hm, I've checked with rawhide that it is possible with the Gtk UI: set it in the entry of Network Spoke to a value, empty string, and then a value again - the value will propagate in the backend on leaving the Network spoke. Also setting empty hostname is possible. There will be empty /etc/hostname on target system.

@adamkankovsky adamkankovsky force-pushed the hostname-handling branch 3 times, most recently from 98850e0 to 120baf1 Compare April 30, 2024 13:29
@adamkankovsky
Copy link
Contributor Author

If I change a value from empty/transient to something, then I am not able to change it back to empty/transient. I think we should allow this (and add to tests if so).

Unfortunately there is a bit of a problem here, because Anaconda doesn't allow you to set an empty hostname, so it won't be possible to fix this without modifying the backend.

Hm, I've checked with rawhide that it is possible with the Gtk UI: set it in the entry of Network Spoke to a value, empty string, and then a value again - the value will propagate in the backend on leaving the Network spoke. Also setting empty hostname is possible. There will be empty /etc/hostname on target system.

@rvykydal So I finally modified the regex to allow empty hostnames and added tests to test it.

const { buttonLabel, buttonVariant, dialogTitleIconVariant, dialogWarning, dialogWarningTitle } = useScenario();

export const ReviewConfigurationConfirmModal = ({
idPrefix,
Copy link
Contributor

Choose a reason for hiding this comment

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

these changes are unrelated and add noise to the commit. Please avoid code-reorganization in feature commits.

@KKoukiou KKoukiou force-pushed the hostname-handling branch 3 times, most recently from baf4085 to 58abdc4 Compare May 7, 2024 14:45
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

I split out the hostname related code to seperate component and file for readability purposes. Also the groupping of the description elements should be seperate commit for the same purpose.

@KKoukiou KKoukiou merged commit 4a2cbf9 into rhinstaller:main May 8, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants