-
Notifications
You must be signed in to change notification settings - Fork 24
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
Bug 2132793: Load and show boot source reference #913
Bug 2132793: Load and show boot source reference #913
Conversation
@upalatucci: This pull request references Bugzilla bug 2132793, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: lkladnit. Note that only kubevirt-ui members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@upalatucci: This pull request references Bugzilla bug 2132793, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: lkladnit. Note that only kubevirt-ui members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
2515d9e
to
325d7a9
Compare
@upalatucci: This pull request references Bugzilla bug 2132793, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: lkladnit. Note that only kubevirt-ui members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@upalatucci: This pull request references Bugzilla bug 2132793, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: lkladnit. Note that only kubevirt-ui members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments to make sure about some code changes. Anyway, great work, Ugo! ❇️
...ews/templates/actions/components/PersistentVolumeClaimSelect/PersistentVolumeClaimSelect.tsx
Show resolved
Hide resolved
try { | ||
dataVolume = await getDataVolume(dataSourcePVCName, dataSourcePVCNamespace); | ||
} catch (error) { | ||
console.warn(error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we really want console.warn
here, if it's about catching error.
Also maybe we want to display the error/warning the different way - visibly for the user - as in other places in the code... Can you, please, explain the reason of using console.warn
here? Just for better understanding of this change :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used warn because deep source wants something in the catch. If getDataVolume
throw an error means that there is no DV. It's not an error. What do you think? Should I use console.error
anyways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe console.warn
+ some comment in the code about why we use warn
and not error
, could be sufficient :) Or maybe also console.error
+ some comment about that it is not really an error.
But now I am wondering, what if getDataVolume
really fails, and not because there was no DV... Can this ever happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if the client lost connection? So also the creation will fail.
325d7a9
to
f964662
Compare
f964662
to
9e5adbd
Compare
); | ||
|
||
if (!loaded) return <PersistentVolumeClainSelectSkeleton />; | ||
if (!projectsLoaded) return <PersistentVolumeClainSelectSkeleton />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo in that component name. It should be 'Claim' not 'Clain'. We should probably fix that in a separate PR.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hstastna, pcbailey, upalatucci The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@upalatucci: All pull requests linked via external trackers have merged: Bugzilla bug 2132793 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
📝 Description
Cause
The EditBootSourceModal does not fetch the dataVolume and does not show the dataVolume source.
Solution
Fetch DV from DataSource and get from it the specs. Populate the form with specs data and be able to edit them. On submission, edit the DV
Most of the changes were made in the SelectSource.
The previous select source was not accepting a state or an initial state so was impossible to solve the bug without touching it.
Also in the component, there were a giant
useEffect
runner at every update.Now the SelectSource accepts a state (data volume spec) and all the component states were transformed into constants dependent on that state. There is just one callback when the volume or source type change
🎥 Demo
Screencast.from.11-10-2022.10.46.52.webm