-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: enable AWS integrations for all #7111
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.
👍 Looks good to me! Reviewed everything up to 36d85e0 in 57 seconds
More details
- Looked at
44
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/pages/Integrations/IntegrationsList.tsx:117
- Draft comment:
Avoid inline styles in React components. Consider moving the style definition to a CSS class or using a styled component. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. frontend/src/pages/Integrations/IntegrationsList.tsx:117
- Draft comment:
Avoid using inline styles. Move the style={{ display: 'flex', gap: '10px' }} to a CSS class in the stylesheet. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/src/pages/Integrations/IntegrationsList.tsx:50
- Draft comment:
Consider caching searchTerm.toLowerCase() in a variable to avoid recalculating it multiple times, which improves code clarity and may offer slight performance benefits. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. frontend/src/pages/Integrations/IntegrationsList.tsx:47
- Draft comment:
The removal of the feature flag check for AWS integration fits the PR description. Make sure any legacy feature flag code (and related unused imports) is cleaned up across the codebase. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_63jL8QqamV584YIp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
…n ctx of a cloud account
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.
👍 Looks good to me! Incremental review on ec47d09 in 42 seconds
More details
- Looked at
44
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/CloudIntegrationPage/ServicesSection/ServiceDetails.tsx:118
- Draft comment:
The onClick handler is duplicated in both branches. Consider extracting it into a variable or a separate function to enhance clarity and reduce duplication. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. frontend/src/container/CloudIntegrationPage/ServicesSection/ServiceDetails.tsx:118
- Draft comment:
The refactored conditional now wraps the entire ternary in a check for cloudAccountId. Previously, even if cloudAccountId was falsy the 'Enable Service' button would render. This change hides the button when cloudAccountId is absent. If this behavior is intentional, please add a clarifying comment. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to confirm their intention by suggesting to add a clarifying comment if the behavior is intentional. This violates the rule against asking for confirmation of intention.
Workflow ID: wflow_RiPM8wBkhKnGsU4v
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 a small comment 🙂
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.
👍 Looks good to me! Incremental review on 1aeb5c1 in 54 seconds
More details
- Looked at
66
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. ee/query-service/model/plans.go:157
- Draft comment:
Removed AWSIntegration flag blocks from all plans. Confirm that AWS integration is now enabled by default and that no dependent code expects this flag. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%
<= threshold50%
This comment is asking the PR author to confirm that AWS integration is enabled by default and that no dependent code expects the flag. This is a request for confirmation, which is not allowed by the rules. However, it is also pointing out a potential issue with the removal of the flag, which could be useful.
2. frontend/src/constants/features.ts:24
- Draft comment:
Removed AWS_INTEGRATION enum entry. Ensure no UI parts reference this flag. - Reason this comment was not posted:
Marked as duplicate.
3. pkg/query-service/model/featureSet.go:24
- Draft comment:
Removed AWSIntegration constant. Verify backend code is updated to handle AWS integration accordingly. - Reason this comment was not posted:
Marked as duplicate.
4. ee/query-service/model/plans.go:157
- Draft comment:
Removed AWSIntegration toggle from plan definitions; ensure no dependent logic expects a plan-specific flag now. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. frontend/src/constants/features.ts:24
- Draft comment:
Removed AWS_INTEGRATION enum key; confirm that UI code no longer references this feature toggle. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. pkg/query-service/model/featureSet.go:24
- Draft comment:
Removed AWSIntegration constant; ensure all references are updated for the new global AWS integration approach. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_8Q97XarACQAQmzwQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Adds AWS integration to integrations list for all tenants
Related Issues / PR's
contributes to https://github.com/SigNoz/engineering-pod/issues/2023
Important
Enable AWS integration for all tenants by default, removing feature flag checks and conditions.
IntegrationsList.tsx
.AWSIntegration
feature fromBasicPlan
,ProPlan
, andEnterprisePlan
inplans.go
.AWSIntegration
constant fromfeatureSet.go
.AWS_INTEGRATION
fromFeatureKeys
infeatures.ts
.ServiceDetails.tsx
to simplify button rendering logic.IntegrationsList.tsx
.This description was created by
for 1aeb5c1. It will automatically update as commits are pushed.