Skip to content

Conversation

@iamrajjoshi
Copy link
Collaborator

I recently merged a migration to add prevent_superuser_access to the Organization bitflag and now need to add it to Hybrid Cloud services to sync since I need it to exist on RpcOrganization.

In this PR, I also add the disable_member_project_creation flag since it existed above the new flag I created.

My flag is still not used in our logic, but the disable_member_project_creation is currently being used, so we need to make sure this pr doesn't mess around with its value.

@iamrajjoshi iamrajjoshi requested a review from a team July 24, 2024 21:11
@iamrajjoshi iamrajjoshi self-assigned this Jul 24, 2024
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 24, 2024
@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0746_add_bitflags_to_hybrid_cloud.py ()

--
-- Custom state/database change combination
--

                    ALTER TABLE "sentry_organizationmapping"
                    ADD COLUMN "disable_member_project_creation" boolean NOT NULL DEFAULT false,
                    ADD COLUMN "prevent_superuser_access" boolean NOT NULL DEFAULT false;

@iamrajjoshi iamrajjoshi marked this pull request as ready for review July 24, 2024 21:45
@iamrajjoshi iamrajjoshi requested review from a team as code owners July 24, 2024 21:45
Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

Migration lgtm

@iamrajjoshi
Copy link
Collaborator Author

Migration lgtm

could i pytest.skip the failing tests? i think they are happening because the function i updated expects my migration's columns to exist.

@iamrajjoshi iamrajjoshi changed the title feat(data-secrecy): Migration to Add BitFlags to Hybrid Cloud Services feat(data-secrecy): Migration to Add Bit Flags to Hybrid Cloud Services Jul 24, 2024
@wedamija
Copy link
Member

Migration lgtm

could i pytest.skip the failing tests? i think they are happening because the function i updated expects my migration's columns to exist.

Yeah, it's fine to skip migration tests

@codecov
Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.17%. Comparing base (7f1026a) to head (5138da8).
Report is 102 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #74891       +/-   ##
===========================================
+ Coverage   57.10%   78.17%   +21.07%     
===========================================
  Files        6728     6742       +14     
  Lines      300098   300868      +770     
  Branches    51618    51737      +119     
===========================================
+ Hits       171376   235210    +63834     
+ Misses     123944    59319    -64625     
- Partials     4778     6339     +1561     
Files Coverage Δ
.../hybridcloud/services/organization_mapping/impl.py 81.35% <ø> (+23.72%) ⬆️
...hybridcloud/services/organization_mapping/model.py 100.00% <100.00%> (ø)
...ybridcloud/services/organization_mapping/serial.py 100.00% <ø> (ø)
src/sentry/models/organizationmapping.py 74.07% <100.00%> (+0.99%) ⬆️
...entry/organizations/services/organization/model.py 89.90% <100.00%> (+17.22%) ⬆️

... and 2017 files with indirect coverage changes

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Looks safe to deploy to me. As regions are updated, they will start sending the new flag keys, and once control updates the flag state will start being updated. On the read side, the default values in RpcOrganizationMappingFlags will the columns in.

sort="date",
)

@pytest.mark.skip(reason="old migration test")
Copy link
Member

Choose a reason for hiding this comment

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

If these tests don't work anymore, why not delete them?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's helpful to keep some around as an example, but we could delete old skipped tests after a while

@iamrajjoshi iamrajjoshi merged commit 70f93f4 into master Jul 25, 2024
@iamrajjoshi iamrajjoshi deleted the raj/ds/hc-mig branch July 25, 2024 14:50
Christinarlong pushed a commit that referenced this pull request Jul 26, 2024
…es (#74891)

I recently merged a
[migration](#74700) to add
`prevent_superuser_access` to the Organization bitflag and now need to
add it to Hybrid Cloud services to sync since I need it to exist on
`RpcOrganization`.

In this PR, I also add the `disable_member_project_creation` flag since
it existed above the new flag I created.

My flag is still not used in our logic, but the
`disable_member_project_creation` is currently being used, so we need to
make sure this pr doesn't mess around with its value.
@github-actions github-actions bot locked and limited conversation to collaborators Aug 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants