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 DNS search and ignore options from kickstart #4519

Merged

Conversation

VladimirSlavik
Copy link
Contributor

@VladimirSlavik VladimirSlavik commented Jan 23, 2023

This is the anaconda part.

Blocked by waiting on pykickstart/pykickstart#431, which in turn is blocked by rhbz#1656662.

TODOs:

@VladimirSlavik VladimirSlavik added port to RHEL8 manual testing required This issue can't be merged without manual testing blocked Don't merge this pull request! notable change Important changes like API change, behavior change... port to RHEL9 The pull request needs to be ported to RHEL 9. f38 Fedora 38 labels Jan 23, 2023
@VladimirSlavik VladimirSlavik force-pushed the master-network-dns-domains branch 3 times, most recently from c0d060e to 1d4ea2f Compare February 1, 2023 18:53
@VladimirSlavik VladimirSlavik removed the manual testing required This issue can't be merged without manual testing label Feb 1, 2023
@VladimirSlavik
Copy link
Contributor Author

Tested manually to work OK.

Blocked on pykickstart merge and release.

@VladimirSlavik
Copy link
Contributor Author

add tests

I don't think tests are actually needed. Pretty much none of the functions in the module have any :(

@VladimirSlavik VladimirSlavik marked this pull request as ready for review February 6, 2023 13:40
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.

I think we should also include the option in generated kickstart:

def get_kickstart_network_data(connection, nm_client, network_data_class):
.

@rvykydal
Copy link
Contributor

rvykydal commented Feb 8, 2023

add tests

I don't think tests are actually needed. Pretty much none of the functions in the module have any :(

I wouldn't insist on adding them in scope of this PR. We should add a new issue for adding unit tests for nm-client.py. Or maybe adding a test for update_connection_ip_settings_from_ksdata in scope of this BZ could be a nice start.

Also we might want to add a kickstart test for the feature (or update an existing one but I didn't find any suitable in TESTTYPE="network" set). It would just do something like

check_device_config_value @KSTEST_NETDEV2@ DNS1 10.43.26.2 ipv4 dns "10.43.26.2;"

is done in https://github.com/rhinstaller/kickstart-tests/blob/eb975b25d14b75916bf2ea5d2c25af51080db305/network-autoconnections-httpks.ks.in#L100
A question is if we want a new test specific for this feature (having resources in mind); or if we use some existing, but a bit unrelated, one, like the network-autoconnections-httpks; or if we create some generic test for network options of similar nature. I'd maybe prefer the last option.

@VladimirSlavik
Copy link
Contributor Author

Added the ks generation part, now (re-)testing manually

@VladimirSlavik
Copy link
Contributor Author

Manual testing succeeded but needed rebasing ^

@VladimirSlavik
Copy link
Contributor Author

Or maybe adding a test for update_connection_ip_settings_from_ksdata in scope of this BZ could be a nice start.

Sounds good to me.

@rvykydal
Copy link
Contributor

Or maybe adding a test for update_connection_ip_settings_from_ksdata in scope of this BZ could be a nice start.

Sounds good to me.

Thank you, looks good to me so far. I'll wait for the tests and finish the review.

@VladimirSlavik
Copy link
Contributor Author

The tests won't run without pykickstart, so I'd perhaps get that merged?

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, thank you!

@VladimirSlavik VladimirSlavik removed the blocked Don't merge this pull request! label Feb 16, 2023
@VladimirSlavik
Copy link
Contributor Author

No longer blocked by pykickstart at all, except for propagation to mirrors.

@VladimirSlavik VladimirSlavik added port to Fedora f39 and removed f38 Fedora 38 labels Feb 17, 2023
@VladimirSlavik
Copy link
Contributor Author

/kickstart-test --testtype smoke

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.

@VladimirSlavik VladimirSlavik merged commit b2b05a4 into rhinstaller:master Feb 20, 2023
@VladimirSlavik VladimirSlavik deleted the master-network-dns-domains branch February 20, 2023 16:48
@VladimirSlavik VladimirSlavik removed the port to RHEL9 The pull request needs to be ported to RHEL 9. label Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f39 notable change Important changes like API change, behavior change... port to RHEL8
2 participants