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: Fix incorrect format when adding existing disk to VM #13437

Merged
merged 3 commits into from Feb 18, 2020

Conversation

skobyda
Copy link
Contributor

@skobyda skobyda commented Jan 23, 2020

If you try to add disk to VM, and switch to "Use existing" without
changing anything else in the UI (keep default pool and volume),
the format is not updated into correct value.
Fix this.

https://bugzilla.redhat.com/show_bug.cgi?id=1792319

@skobyda skobyda requested a review from KKoukiou January 23, 2020 11:58
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.

This broke testAddDisk

@skobyda skobyda force-pushed the fixSharedDiskBug branch 2 times, most recently from 8dbe943 to 4c2bc93 Compare January 30, 2020 12:03
@@ -397,6 +397,10 @@ export class AddDiskModalBody extends React.Component {
onAddClicked() {
const { vm, dispatch, provider, close, vms, storagePools } = this.props;

// validate
if (!this.state.storagePoolName)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should actually never be non-set.
If it does it means we have some bug somewhere in the contructor of the DiskAdd component, where the initially selected storage pool should be set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually shows when user does not have any storage pools on their host. Then instead of dropdown we show text: No Storage Pools available. In that case, state.storagePoolName is undefined, however we do not prevent user from clicking "Add" button and calling attachDisk action, and it results in some error.
This should prevent from attachDisk action being called.

@@ -433,6 +437,9 @@ export class AddDiskModalBody extends React.Component {
}

// use existing volume
if (!this.state.existingVolumeName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for the preselected options comment as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same, is user has some pool with no volumes, this.state.existingVolumeName is undefined, but we do not prevent them from clicking "Add" and attachDisk action being called.

@KKoukiou
Copy link
Contributor

@skobyda ubuntu-1804 is failing on the changed code

If you try to add disk to VM, and switch to "Use existing" without
changing anything else in the UI (keep default pool and volume),
the format is not updated into correct value.
Fix this.

https://bugzilla.redhat.com/show_bug.cgi?id=1792319

Closes: cockpit-project#13437
@martinpitt martinpitt added the release-blocker Targetted for next release label Feb 18, 2020
@martinpitt martinpitt merged commit e8c149d into cockpit-project:master Feb 18, 2020
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.

None yet

3 participants