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

Add support for EIPs associated with secondary ENIs #1513

Merged
merged 8 commits into from
Nov 13, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
test something
Brian Tucker committed Nov 8, 2023
commit 470ff15c90b71f2b4fcc07f53b0f9d1d947d5976
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did the ClusterSampleData change?

Copy link
Author

@brituck brituck Nov 9, 2023

Choose a reason for hiding this comment

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

In the update to Mockito 3.4.0 the ArgumentMatcher for anyString() changed from accepting "any string or null" to "any string". In the testStatusUpdate() and testDeleteStatusOverride() tests in PeerReplicationResourceTest, the provided parameter for status in the downstream instanceResource.statusUpdate() call is null, which results in the Mock not matching and our response returning null.

There are two solutions to this:

  1. Refactor the fields in the returned ReplicationInstance (current change in this PR) to correctly capture the intent of the statusUpdate() and deleteStatusUpdate() calls.

    The current master branch calls statusUpdate(String newStatus = null, String isReplication = "true", String lastDirtyTimestamp = "12345678") and deleteStatusUpdate(String isReplication = "true", String newStatus = null, String lastDirtyTimestamp = "12345678"), which to me seems odd that we'd set newStatus = null in each rather than an actual status.
  2. Change the mock expression for statusUpdate() and deleteStatusUpdate() to isNull() for the parameters that return null.

I prefer the first option as I believe it's the more correct change, but I'm happy to go with 2) if we want to minimize the changes in this PR.

Original file line number Diff line number Diff line change
@@ -99,9 +99,9 @@ public static ReplicationInstance newReplicationInstanceOf(Action action, Instan
instance.getAppName(),
instance.getId(),
System.currentTimeMillis(),
null,
InstanceStatus.OUT_OF_SERVICE.name(),
null,
null,
action
);
case DeleteStatusOverride:
@@ -110,7 +110,7 @@ public static ReplicationInstance newReplicationInstanceOf(Action action, Instan
instance.getId(),
System.currentTimeMillis(),
InstanceStatus.OUT_OF_SERVICE.name(),
InstanceStatus.UP.name(),
null,
null,
action
);