Bug 2181323: Select 'Size' automatically if 'Default InstanceType' chosen #1215
Conversation
…osen Select the 1st of the options in 'Size' dropdown if already some 'Default InstanceType' was chosen. Prevent missing 'instancetype.kubevirt.io/default-instancetype' label in the created bootable volume in case of not choosing 'Size' in 'Add volume' modal. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2181323
|
@hstastna: This pull request references Bugzilla bug 2181323, 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. |
|
@avivtur @metalice @pcbailey @upalatucci @vojtechszocs please review |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avivtur, hstastna 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 |
|
@hstastna: All pull requests linked via external trackers have merged: Bugzilla bug 2181323 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. |
| @@ -30,6 +30,7 @@ const InstanceTypeSelect: FC<InstanceTypeSelectProps> = ({ setBootableVolumeFiel | |||
| const [category, setCategory] = useState<string>(); | |||
| const [categorySize, setCategorySize] = useState<string>(); | |||
|
|
|||
| // update options for 'Size' dropdown according to chosen 'category' in 'Default InstanceType' dropdown | |||
There was a problem hiding this comment.
Usually, we are not commenting on our code as code should be self-explanatory, and comments will make your code longer. We will need to maintain more things in the file, and it could lead to more work as continues to update comments or mistakes when commenting wrong. But the most crucial reason is self-explanatory. We do add comments for stuff that aren't trivial at all, or we use some kind of value that is probably unknown to the next developer why we chose this value.
I don't see any of the cases here.
Another thing, I see that you didn't use any ?. on labels or [0] or instanceTypes, are you sure they are always 100% guaranteed?
There was a problem hiding this comment.
IMHO this place worth commenting as it isn't so explanatory in this place, I had troubles to understand what was going on there, even when I pretty knew how it works/how it should work in this place. IMHO I'd use different names for many variables there, those are not self explanatory/100% correct, IMHO, but wasn't brave enough to change that, it would require many more changes in more places, more people involved to agree, I didn't want to make it worse to them, to make the code they've added less readable for them, so I've better added some comment, which can be useful to anyone.
A few comments is nothing bad in the code, it helps to a dev to better understand the code. I'd like to see even more comments in our code, it would make many things easier.. We always can remove what's not relevant or if we make this code even better with time. With tools we use, the code is always simplified, optimized and all the comments are removed by that process, I don't see anything to worry about, regarding "maintaining more things in the file" or regarding that it would "make your code longer". IMO it gives us much more than takes from us.
One more thing: in my previous experiences on various projects at RH I was strongly encouraged to write comments to the code when I considered it useful. We had way more comments in the code, it made things much easier, especially to newcomers..
Of course, too many and unnecessary comments is not good, it's better to look for a reasonable balance in that.
Another thing, I see that you didn't use any ?. on labels or [0] or instanceTypes, are you sure they are always 100% guaranteed?
Yes. I am sure. I've tested various scenarios. I access the object created by us, so there is no reason to use ?. And if it would do 💣 in some special case, it would be only good, because we would realize something is wrong with our code. And that's good. We would be able to fix it asap instead of not seeing something is missing. Check what categoryDetailsMap is. Or test this code by yourself. IMO that's the best option anyway, to make sure in practice.
There was a problem hiding this comment.
IMHO this place worth commenting as it isn't so explanatory in this place, I had troubles to understand what was going on there, even when I pretty knew how it works/how it should work in this place. IMHO I'd use different names for many variables there, those are not self explanatory/100% correct, IMHO, but wasn't brave enough to change that, it would require many more changes in more places, more people involved to agree, I didn't want to make it worse to them, to make the code they've added less readable for them, so I've better added some comment, which can be useful to anyone.
A few comments is nothing bad in the code, it helps to a dev to better understand the code. I'd like to see even more comments in our code, it would make many things easier.. We always can remove what's not relevant or if we make this code even better with time. With tools we use, the code is always simplified, optimized and all the comments are removed by that process, I don't see anything to worry about, regarding "maintaining more things in the file" or regarding that it would "make your code longer". IMO it gives us much more than takes from us.
One more thing: in my previous experiences on various projects at RH I was strongly encouraged to write comments to the code when I considered it useful. We had way more comments in the code, it made things much easier, especially to newcomers..
Of course, too many and unnecessary comments is not good, it's better to look for a reasonable balance in that.
Another thing, I see that you didn't use any ?. on labels or [0] or instanceTypes, are you sure they are always 100% guaranteed?
Yes. I am sure. I've tested various scenarios. I access the object created by us, so there is no reason to use ?. And if it would do 💣 in some special case, it would be only good, because we would realize something is wrong with our code. And that's good. We would be able to fix it asap instead of not seeing something is missing. Check what
categoryDetailsMapis. Or test this code by yourself. IMO that's the best option anyway, to make sure in practice.
Please read
https://levelup.gitconnected.com/please-dont-comment-your-code-d0830785bdc9
https://www.freecodecamp.org/news/code-comments-the-good-the-bad-and-the-ugly-be9cc65fbf83/
https://towardsdatascience.com/why-good-codes-dont-need-comments-92f58de19ad2
The code is self-explanatory - and yes, if you think names of var's are misleading, please fix it - this obviously makes the code better.
About finding and getting a boom - as talked about and explained, it's the worst user experience a customer can have. we should ALWAYS avoid it in UI. it's not a BE that can have fallback processes, here we are crashing!
📝 Description
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2181323
Select the 1st of the options in Size dropdown if already some Default InstanceType was chosen. Prevent missing 'instancetype.kubevirt.io/default-instancetype' label in the created bootable volume's yaml in case of not choosing Size in Add volume modal, because the correctly set instanceType label requires both parts defined. It does not make sense to only choose Default InstanceType without size.
With this change, also align with the actual behavior in Edit modal accessible in Bootable volumes list. Save one user's click when automatically selecting the 1st option from sizes, if they already choose something from Default InstanceType dropdown. They still can choose some other option if they want.
🎥 Demo
Before:
it's possible to add a bootable volume with only Default InstanceType chosen, without Size => label missing:
instance_before.mp4
After:
'Size' automatically set if 'Default InstanceType' chosen by the user => label correctly set:
instance_after.mp4