-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix an issue where clusteractuator.Delete(...) would never succeed if the firewall rules it is attempting to delete do not exist. Move GCEClientComputeServieMock to an appropriate shared location. #378
Fix an issue where clusteractuator.Delete(...) would never succeed if the firewall rules it is attempting to delete do not exist. Move GCEClientComputeServieMock to an appropriate shared location. #378
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: spew 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 |
a520de6
to
84930c4
Compare
/assign @mkjelland |
/assign @roberthbailey |
84930c4
to
22594f7
Compare
@@ -0,0 +1,96 @@ | |||
package google_test | |||
|
|||
import ( |
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.
if you only need a single import you can put it on one line instead of in a block
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.
Fixed.
@@ -0,0 +1,68 @@ | |||
/* |
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.
You need to fix this copyright now that we have the verify boilerplate script (it needs some blank lines).
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.
Fixed and added the copyright to the clientcomputeservice_test.go as well where it was missing.
|
||
func newClusterActuator(t *testing.T, params google.ClusterActuatorParams) cluster.Actuator { | ||
t.Helper() | ||
actuator, err := google.NewClusterActuator(params) |
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.
Since this is just making one fn call and checking for an error is there a reason you put it into a separate fn instead of putting it inline?
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.
Just did this out of force of habit since it is useful once there is more than one test (they can all make a one line function call instead of 3 lines of function call & error handling).
… the firewall rules it is attempting to delete do not exist. Move GCEClientComputeServieMock to an appropriate shared location.
22594f7
to
3d90b8e
Compare
lgtm |
/lgtm |
…-fix-cluster-name hack/generate-yaml.sh: fix hard-coded cluster name
What this PR does / why we need it:
This PR fixes an issue where clusteractuator.Delete(...) would never succeed if the firewall rules it is attempting to delete do not exist. Therefore, it handles a NotFound error by assuming the FirewallRule has already been removed by a previous run of Delete(...) or by an external force (like a GCP adminstratro).
It also moves the GCEClientComputeServieMock to an appropriate shared location since it is being used by both the clusteractuator_test.go and machineactuator_test.go.
Release note:
@kubernetes/kube-deploy-reviewers