-
Notifications
You must be signed in to change notification settings - Fork 3
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
Port 6027 delete entities by owner #36
Conversation
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.
Added few comments 🚀
pkg/k8s/controller.go
Outdated
@@ -248,11 +248,35 @@ func (c *Controller) entityHandler(portEntity port.Entity, action EventActionTyp | |||
} | |||
klog.V(0).Infof("Successfully upserted entity '%s' of blueprint '%s'", portEntity.Identifier, portEntity.Blueprint) | |||
case DeleteAction: | |||
err := c.portClient.DeleteEntity(context.Background(), portEntity.Identifier, portEntity.Blueprint, c.portClient.DeleteDependents) | |||
portEntities, err := c.portClient.SearchEntities(context.Background(), port.SearchBody{ |
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.
Extract to a function - CheckIfOwnEntity
pkg/k8s/controller.go
Outdated
Property: "$datasource", | ||
Operator: "contains", | ||
Value: "port-k8s-exporter", | ||
}, |
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 also to add a rule to check that this specific exporter instance is the owner (checking the stateKey), like here:
port-k8s-exporter/pkg/port/cli/entity.go
Line 100 in d726b09
{ |
pkg/k8s/controller.go
Outdated
} | ||
klog.V(0).Infof("Successfully deleted entity '%s' of blueprint '%s'", portEntity.Identifier, portEntity.Blueprint) | ||
} else { | ||
return fmt.Errorf("trying to delete entity that has different owner id: '%s', blueprint: '%s' ", portEntity.Identifier, portEntity.Blueprint) |
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 can print it just as info or warning, but it's not an error because it's a valid case (also errors are being retried, and you don't want it in that case for sure).
@@ -39,7 +39,11 @@ func newFixture(t *testing.T, portClientId string, portClientSecret string, reso | |||
if portClientSecret == "" { | |||
portClientSecret = config.ApplicationConfig.PortClientSecret | |||
} | |||
portClient, err := cli.New(config.ApplicationConfig.PortBaseURL, cli.WithHeader("User-Agent", "port-k8s-exporter/0.1"), | |||
if userAgent == "" { | |||
userAgent = "port-k8s-exporter/0.1" |
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.
Add also the stateKey and add a test for it
pkg/k8s/controller.go
Outdated
} | ||
klog.V(0).Infof("Successfully deleted entity '%s' of blueprint '%s'", portEntity.Identifier, portEntity.Blueprint) | ||
} else { | ||
klog.Warningf("trying to delete entity but didn't find it in port with k8s ownership", portEntity.Identifier, portEntity.Blueprint) |
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.
klog.Warningf("trying to delete entity but didn't find it in port with k8s ownership", portEntity.Identifier, portEntity.Blueprint) | |
klog.Warningf("trying to delete entity but didn't find it in port with this exporter ownership", portEntity.Identifier, portEntity.Blueprint) |
pkg/k8s/controller_test.go
Outdated
item := EventItem{Key: getKey(d, t), ActionType: DeleteAction} | ||
|
||
f := newFixture(t, "", "", resource, objects) | ||
f := newFixture(t, "", "", fmt.Sprintf("statekey/%s", config.ApplicationConfig.StateKey)+"port-k8s-exporter", resource, objects) |
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.
Can you construct the user-agent like in the code?
fmt.Sprintf("port-k8s-exporter/0.1 (statekey/%s)", applicationConfig.StateKey)
Value: "port-k8s-exporter", | ||
}, | ||
{ | ||
Property: "$identifier", |
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 forgot that you also must check the blueprint identifier, because entity identifier is not unique across blueprints
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.
Good catch!
https://getport.atlassian.net/browse/PORT-6027