Update patch set 15
Patch Set 15: 2:46:17 AM - _rajat_: Hi Eric, 2:46:17 AM - _rajat_: I have some regarding your review comment in 2:46:17 AM - _rajat_: https://review.opendev.org/#/c/678179/15 2:48:10 AM - _rajat_: as cold migration uses resize flow only and test for that has been modified for cold migration also. 2:48:10 AM - _rajat_: can you suggest what else need to be done Assume you're talking about this: > IMO you shouldn't consider merging this until the oot CI is passing > *at least* regressively. Really you should make sure it is running > (and passing) some cold migration tests. You've updated the unit tests, and that's fine. But it's easy to make unit tests pass without in any way proving that the feature *works*. At a minimum with a driver change like this you need to prove that it does not regress existing functionality. As noted above, unit tests are not sufficient for this. My main gripe here is that the PowerVM CI hasn't posted results on this patch, ever. So at the very least that needs to happen before you should consider merging. But ideally you should *also* add some specific cold migration tests to the CI. Not sure if this... 2:48:10 AM - _rajat_: as cold migration uses resize flow only and test for that has been modified for cold migration also. ...was referring to the unit tests updated in the patch or to the CI. If the former, see my comments above. Otherwise: If this patch is needed at all, it must be because cold migration wasn't working (properly) before. Which means you should be able to demonstrate at least one test scenario in the CI that fails without the patch, and succeeds with it. All of that said, this project (despite its openness) is owned by IBM, and since I'm no longer with IBM, you should not feel obligated to treat my reviews as blocking (and I promise not to use my -2 powers if you want to continue keeping me on the core team). Patch-set: 15
This commit is contained in:
Gerrit User 14070
Gerrit Code Review
parent
53b0a77a44
commit
63f43a2109