Skip to content
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

fix: org domain #7148

Merged
merged 2 commits into from
Feb 18, 2025
Merged

fix: org domain #7148

merged 2 commits into from
Feb 18, 2025

Conversation

nityanandagohain
Copy link
Member

@nityanandagohain nityanandagohain commented Feb 18, 2025

Fixes the updated_at type of org domain


Important

Adds SQL migration to modify updated_at column in org_domains table and updates provider configuration.

  • SQL Migration:
    • Adds 012_modify_org_domain.go to modify updated_at column in org_domains table.
    • Renames updated_at to updated_at_old, adds new updated_at as INTEGER, and updates values.
    • Drops updated_at_old column after migration.
  • Provider Configuration:
    • Adds NewModifyOrgDomainFactory() to SQLMigrationProviderFactories in provider.go.

This description was created by Ellipsis for cf6c549. It will automatically update as commits are pushed.

@github-actions github-actions bot added the bug Something isn't working label Feb 18, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 cf6c549 in 1 minute and 6 seconds

More details
  • Looked at 29 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. pkg/sqlmigration/012_modify_org_domain.go:29
  • Draft comment:
    Removed SQLite-only condition. Ensure this migration works for all DB dialects or document any assumptions.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the author to ensure that the migration works for all DB dialects or document assumptions. This falls under asking the author to ensure behavior, which is against the rules.
2. pkg/sqlmigration/012_modify_org_domain.go:29
  • Draft comment:
    Removed the SQLite-only check so the migration now runs on all DBs. Ensure that all ALTER TABLE statements (RENAME, ADD, DROP, UPDATE) are supported across your target SQL dialects.
  • Reason this comment was not posted:
    Marked as duplicate.
3. pkg/sqlmigration/012_modify_org_domain.go:42
  • Draft comment:
    Removed inline comments describing constraint and update behavior. Confirm that sufficient documentation remains so future maintainers understand the migration intent.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_9xRqfQV5RXEx6j4H


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 2bb4726 in 2 minutes and 45 seconds

More details
  • Looked at 89 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. pkg/sqlmigration/012_modify_org_domain.go:31
  • Draft comment:
    The migration only applies for sqlite. Add a comment explaining why this operation is limited to sqlite or consider handling other dialects if needed.
  • Reason this comment was not posted:
    Marked as duplicate.
2. pkg/signoz/provider.go:64
  • Draft comment:
    Ensure the new ModifyOrgDomain migration is correctly ordered and only applies to old SQLite databases as intended.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
3. pkg/sqlmigration/012_modify_org_domain.go:31
  • Draft comment:
    Document why this migration only runs for SQLite (e.g. legacy schema issue) to aid future maintainers.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. pkg/sqlmigration/012_modify_org_domain.go:43
  • Draft comment:
    Verify that the SQLite version in use supports ALTER TABLE operations like RENAME COLUMN and DROP COLUMN.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the PR author to verify compatibility of SQLite version with certain operations. It violates the rule against asking the author to confirm or verify things.
5. pkg/sqlmigration/012_modify_org_domain.go:48
  • Draft comment:
    Clarify the comment about not adding a NOT NULL constraint—explain if this is due to SQLite limitations.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
6. pkg/sqlmigration/012_modify_org_domain.go:69
  • Draft comment:
    Down migration is unimplemented. Consider adding a comment to indicate that this migration is irreversible.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_geWamaIsVVFi4nkT


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@grandwizard28 grandwizard28 enabled auto-merge (squash) February 18, 2025 16:07
@grandwizard28 grandwizard28 merged commit ecbc4ac into main Feb 18, 2025
16 of 17 checks passed
@grandwizard28 grandwizard28 deleted the fix/org_domain branch February 18, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants