-
Notifications
You must be signed in to change notification settings - Fork 737
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
chore: Setting pubsub annotations using --set in makefile #3160
chore: Setting pubsub annotations using --set in makefile #3160
Conversation
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3160 +/- ##
==========================================
- Coverage 54.48% 54.46% -0.03%
==========================================
Files 134 134
Lines 12329 12329
==========================================
- Hits 6718 6715 -3
- Misses 5117 5120 +3
Partials 494 494
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Why are we setting annotations for a third-party project in our helm file?
IMO, This should be removed or replaced with a project-neutral mechanism for setting annotations (I believe we can already set annotations in the config file?)
PubSub being in alpha would justify the removal
To be clear, we avoid any mention of third-party projects in our configs (Prometheus, cert-manager, etc.), because we don't want to take on the responsibility of making sure they are current. Embedded dependencies are unavoidable in the case of golang libraries (e.g. metrics exporters), but in that case, we also have gomod/semver and (IIRC), don't have any config knobs other than those we implement directly. |
Misread the current state: this proposal is to add pubsub directly to the Helm file (currently the annotations are in the Makefile for testing, which is fine). We should not add these annotations to Helm for the reasons stated above. |
@maxsmythe based on your last comment we are good to merge this change then? since this PR does not add add anything related to dapr annotations in helm file. |
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
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.
LGTM
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.
LGTM
…y-agent#3160) Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
What this PR does / why we need it: Improving logical condition to set dapr annotations when pubsub is enabled and annotations are provided.
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged):Fixes #
Special notes for your reviewer: