-
Notifications
You must be signed in to change notification settings - Fork 43
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
Use provider ID instead of name when sending events #3093
Conversation
if inf.ProviderID == uuid.Nil { | ||
provider, err = e.providerStore.GetByName(ctx, inf.ProjectID, inf.Provider) | ||
} else { | ||
if inf.ProviderID != uuid.Nil { |
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.
Swapped this condition because it is now expected that the Provider ID path will the more commonly used one
@@ -0,0 +1,81 @@ | |||
// Copyright 2024 Stacklok, Inc |
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.
Previously part of the helpers package
@@ -0,0 +1,47 @@ | |||
// Copyright 2024 Stacklok, Inc |
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.
Previously part of the helpers package
@@ -0,0 +1,91 @@ | |||
// Copyright 2024 Stacklok, Inc |
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.
Previously part of the helpers package. Moved under eea, because only the eea code uses 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.
What's the sequencing on this with the previous PR? Do we need one pushed in production and clearing all the previous messages which only had name before we start this one?
My previous PR was part of yesterday evening's deploy to prod, so this PR is safe to merge. |
e75757b
to
bf92ab0
Compare
Relates to #3071 This PR changes any code which creates an EntityInfoWrapper to use Provider ID instead of name. The name is still left as a field in the event for backwards compatibility purposes, and the consumer can still use the name to create the provider instance. However, the name is no longer populated. Some other refactoring was carried out, specifically moving code from the helper package to more appropriate packages.
Did some local verification |
Relates to #3071
This PR changes any code which creates an EntityInfoWrapper to use Provider ID instead of name. The name is still left as a field in the event for backwards compatibility purposes, and the consumer can still use the name to create the provider instance. However, the name is no longer populated.
Some other refactoring was carried out, specifically moving code from the helper package to more appropriate packages.
Summary
Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.
Fixes #(related issue)
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: