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

machines: disable pxe booting when a user selected session connection #12393

Merged
merged 1 commit into from
Jul 24, 2019

Conversation

KKoukiou
Copy link
Contributor

@marusak
Copy link
Member

marusak commented Jul 23, 2019

Needs to rebase to master since #12367 changed tests names

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -596,7 +599,7 @@ class CreateVmModal extends React.Component {
this.setState({ [key]: value });
if (this.state.sourceType == PXE_SOURCE) {
// If the installation source mode is PXE refresh the list of available networks
this.onValueChanged('sourceType', PXE_SOURCE);
this.onValueChanged('sourceType', LOCAL_INSTALL_MEDIA_SOURCE);
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ups, I forgot '&& value == LIBVIRT_SESSION_CONNECTION'

Basically when a user switches back to session we need to reset the choice of installation source if before they had chosen PXE, since the selection would not be valid any more.

Copy link
Member

Choose a reason for hiding this comment

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

I see. So the comment should rather say "When changing to session connection, reset media source"?

@martinpitt
Copy link
Member

#12388 introduced almost (but not quite) the same change to Select, so this needs rebasing now.

@KKoukiou KKoukiou force-pushed the pxe-non-system branch 2 times, most recently from 6cb38f9 to 53ac1c4 Compare July 24, 2019 06:54
@KKoukiou KKoukiou added release-blocker Targetted for next release and removed needs-rebase labels Jul 24, 2019
<Select.SelectEntry title={connectionName == 'session' ? _("Network Boot is available only when using System connection") : null}
disabled={connectionName == 'session'}
data={PXE_SOURCE}
key={PXE_SOURCE}>{_("Network Boot (PXE)")}</Select.SelectEntry> }
Copy link
Member

Choose a reason for hiding this comment

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

Missing line break here? (But not important enough to re-push, as long as eslint is happy)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -596,7 +599,7 @@ class CreateVmModal extends React.Component {
this.setState({ [key]: value });
if (this.state.sourceType == PXE_SOURCE) {
// If the installation source mode is PXE refresh the list of available networks
this.onValueChanged('sourceType', PXE_SOURCE);
this.onValueChanged('sourceType', LOCAL_INSTALL_MEDIA_SOURCE);
Copy link
Member

Choose a reason for hiding this comment

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

I see. So the comment should rather say "When changing to session connection, reset media source"?

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks!

@martinpitt martinpitt merged commit 9645822 into cockpit-project:master Jul 24, 2019
@KKoukiou KKoukiou deleted the pxe-non-system branch July 24, 2019 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Targetted for next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants