-
Notifications
You must be signed in to change notification settings - Fork 91
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
Feature/implement triggers #1364
base: feature/trigger-base
Are you sure you want to change the base?
Conversation
Riddhi reopen a PR and point it to feature/trigger-base we're going to consolidate all trigger branches there |
metadata/metadata.go
Outdated
} | ||
|
||
func (resource *triggerResource) Dependencies(lookup ResourceLookup) (ResourceLookup, error) { | ||
// TODO: Change later, do we need to return all the resources that use this trigger? |
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.
the only current validation rule we have is we can't delete triggers that have resources currently, so if it's a performance issue, we only need to know that it has some resources not necessarily all of them.
metadata/metadata.go
Outdated
return nil, fmt.Errorf("resource not of type trigger: %v", err) | ||
} | ||
//asserted_trigger.serialized.Resources = append(asserted_trigger.serialized.Resources, &pb.ResourceID{Resource: &pb.NameVariant{Name: variant.Name, Variant: variant.Variant}, ResourceType: 6}) | ||
err = serv.lookup.Set(trigger.ID(), asserted_trigger) |
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 this creating the trigger? Unsure what we need this for?
return serv.genericCreate(ctx, &triggerResource{trigger}, nil) | ||
} | ||
|
||
func (serv *MetadataServer) AddTrigger(ctx context.Context, tr *pb.TriggerRequest) (*pb.Empty, error) { |
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.
Lets walk through the add and remove when you get the other stuff cleaned up. Theres probably cleaner ways to do these functions
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.
+10 on this, you can delegate the creation to each variant instead of putting all the logic in here
metadata/metadata.go
Outdated
@@ -1892,6 +2256,59 @@ func (serv *MetadataServer) genericCreate(ctx context.Context, res Resource, ini | |||
serv.Logger.Error(errors.WithStack(err)) | |||
return nil, err | |||
} | |||
|
|||
triggers := []string{} |
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.
All this logic can probably be in propogate change. Lets go through this after the rest is cleaned up
09bf966
to
8d70f0b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/trigger-base #1364 +/- ##
=======================================================
Coverage ? 56.49%
=======================================================
Files ? 203
Lines ? 25960
Branches ? 908
=======================================================
Hits ? 14665
Misses ? 9663
Partials ? 1632 ☔ View full report in Codecov by Sentry. |
7f747fb
to
b8902a6
Compare
…plementation of add_trigger
Description
Type of change
Does this correspond to an open issue?
Select type(s) of change
Checklist: