-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix condition in forceNewIfNetworkIPNotUpdatableFunc #11395
Fix condition in forceNewIfNetworkIPNotUpdatableFunc #11395
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @ScottSuarez, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
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.
Is there a way to add a testcase for this to validate behavior going forward?
Overall the change makes sense to me.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 979 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
Yup, will add 2 configs that change the network IP and expect a non-empty plan. Because before the |
case 1:
case 2:
case 3:
This would previously fail and produce a non-empty plan before the changes |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 980 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
Looks like there are unit test failures.
TestComputeInstance_networkIPCustomizedDiff |
…e more restrictive for unit test
Failed due to the OR statement that i added just in case. This is more restrictive and passing the unit test |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 981 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
Resolves issue #16494
This function didn't work before.
d.HasChange will always return
true
in this condition because of this comparasion on thenetwork
fieldRelease Note Template for Downstream PRs (will be copied)