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

Warn on invalid host name in kickstart (#1897514) #3135

Merged

Conversation

VladimirSlavik
Copy link
Contributor

@VladimirSlavik VladimirSlavik commented Feb 2, 2021

Do that instead of crashing.

Resolves: rhbz#1897514

I have no idea where to put that warning? Or maybe I could use the stock pykickstart one. Opinions welcome.

Pinging @jstodola because it changes behavior, from crashing to... something.

@VladimirSlavik VladimirSlavik added master Please, use the `f39` label instead. tracked labels Feb 2, 2021
@VladimirSlavik
Copy link
Contributor Author

/packit build

@VladimirSlavik
Copy link
Contributor Author

/kickstart-test --testtype smoke

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

I have some suggestions.

pyanaconda/modules/network/network.py Outdated Show resolved Hide resolved
pyanaconda/modules/common/errors/general.py Outdated Show resolved Hide resolved
pyanaconda/modules/network/network.py Outdated Show resolved Hide resolved
@poncovka
Copy link
Contributor

poncovka commented Feb 2, 2021

Please, write a unit test for this use case.

@VladimirSlavik VladimirSlavik force-pushed the master-invalid-hostname branch 2 times, most recently from 37a58c3 to b067306 Compare February 2, 2021 20:14
@VladimirSlavik VladimirSlavik added the blocked Don't merge this pull request! label Feb 2, 2021
@VladimirSlavik
Copy link
Contributor Author

VladimirSlavik commented Feb 2, 2021

Waiting for pykickstart PR. If unblocked and to be continued, tests are still TODO.

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

Not doing pykickstart, so not blocked.

@VladimirSlavik VladimirSlavik force-pushed the master-invalid-hostname branch from b067306 to 356a1e2 Compare February 4, 2021 18:05
@VladimirSlavik
Copy link
Contributor Author

/kickstart-test --testtype smoke

@VladimirSlavik
Copy link
Contributor Author

Update: Logic is now in Network.parse, test added.

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

I have some suggestions.

pyanaconda/modules/network/kickstart.py Outdated Show resolved Hide resolved
pyanaconda/modules/network/kickstart.py Outdated Show resolved Hide resolved
Do that instead of crashing with no message.

Resolves: rhbz#1897514
@VladimirSlavik VladimirSlavik force-pushed the master-invalid-hostname branch from 356a1e2 to 4fd7b99 Compare February 5, 2021 16:50
@VladimirSlavik
Copy link
Contributor Author

Thank you, done.

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

It looks great. Thanks!

@poncovka
Copy link
Contributor

poncovka commented Feb 8, 2021

/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 b1d83a5 into rhinstaller:master Feb 8, 2021
@VladimirSlavik VladimirSlavik deleted the master-invalid-hostname branch February 8, 2021 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master Please, use the `f39` label instead.
Development

Successfully merging this pull request may close these issues.

3 participants