feat: regenerate protos from v0.1.123.0 and add slug/logo_url to CreateOrganizationOptions#162
Open
srinivaskarre-sk wants to merge 9 commits into
Open
feat: regenerate protos from v0.1.123.0 and add slug/logo_url to CreateOrganizationOptions#162srinivaskarre-sk wants to merge 9 commits into
srinivaskarre-sk wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBumps proto ref to v0.1.123.0 and regenerates Python protobuf/gRPC bindings: adds OrganizationCustomizations and logo_url fields, introduces TestUser and Environment Get/UpdateEnvironmentTestUser RPCs, updates descriptors across modules, and adds tests for organization logo/slug persistence. ChangesProto v0.1.123.0 Regeneration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Line 14: The Makefile currently defaults PROTO_REPO_URL to the SSH form which
breaks CI and fresh dev setups; change the PROTO_REPO_URL default to the HTTPS
repo URL (https://github.com/scalekit-inc/scalekit.git) while keeping the
ability to override it via environment or make invocation (i.e., leave the
variable assignment as an overrideable Makefile variable like PROTO_REPO_URL ?=
... or similar), and document briefly in a comment that SSH is supported by
overriding PROTO_REPO_URL to git@github.com:... for devs with SSH access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcffaf10-7d3e-4320-819e-ba4cb9ea110e
📒 Files selected for processing (15)
Makefilescalekit/v1/auth/auth_pb2.pyscalekit/v1/auth/auth_pb2.pyiscalekit/v1/clients/clients_pb2.pyscalekit/v1/commons/commons_pb2.pyscalekit/v1/commons/commons_pb2.pyiscalekit/v1/connected_accounts/connected_accounts_pb2.pyscalekit/v1/domains/domains_pb2.pyscalekit/v1/environments/environments_pb2.pyscalekit/v1/environments/environments_pb2.pyiscalekit/v1/environments/environments_pb2_grpc.pyscalekit/v1/organizations/organizations_pb2.pyscalekit/v1/organizations/organizations_pb2.pyiscalekit/v1/roles/roles_pb2.pyscalekit/v1/tools/tools_pb2.py
|
|
||
| PROTO_REPO_URL := https://github.com/scalekit-inc/scalekit.git | ||
| PROTO_REF ?= v0.1.121.2 | ||
| PROTO_REPO_URL := git@github.com:scalekit-inc/scalekit.git |
There was a problem hiding this comment.
Defaulting to SSH repo URL can break make generate in CI and fresh dev environments.
Using git@github.com:... as the default requires SSH key setup and repo access; this can fail for contributors and automation that previously worked with HTTPS. Prefer HTTPS as the default and keep SSH as an override.
Suggested change
-PROTO_REPO_URL := git@github.com:scalekit-inc/scalekit.git
+PROTO_REPO_URL ?= https://github.com/scalekit-inc/scalekit.git📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| PROTO_REPO_URL := git@github.com:scalekit-inc/scalekit.git | |
| PROTO_REPO_URL ?= https://github.com/scalekit-inc/scalekit.git |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` at line 14, The Makefile currently defaults PROTO_REPO_URL to the
SSH form which breaks CI and fresh dev setups; change the PROTO_REPO_URL default
to the HTTPS repo URL (https://github.com/scalekit-inc/scalekit.git) while
keeping the ability to override it via environment or make invocation (i.e.,
leave the variable assignment as an overrideable Makefile variable like
PROTO_REPO_URL ?= ... or similar), and document briefly in a comment that SSH is
supported by overriding PROTO_REPO_URL to git@github.com:... for devs with SSH
access.
Tests create and update paths for the new slug and logo_url proto fields on CreateOrganization and UpdateOrganization. logo_url tests skip gracefully when the backend has not yet deployed support for that field.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_organization_slug_logo.py`:
- Line 41: The tests use a hardcoded slug variable (slug = 'app.acmecorp.com')
which can collide across parallel/integration runs; update the tests in
tests/test_organization_slug_logo.py to generate a unique slug for each run
(e.g., append or replace with a UUID or timestamp) wherever slug is defined
(including the occurrences around lines 41 and 77-79) so each test creates/uses
an isolated org slug; ensure the generated slug still conforms to any validation
rules used by functions under test (e.g., create_organization or similar helper
functions) so assertions remain valid.
- Around line 11-13: TestOrganizationSlugLogo overrides tearDown without calling
BaseTest.tearDown, skipping the CI-required delay in BaseTest.tearDown and
risking flakiness; update TestOrganizationSlugLogo.tearDown to call
super().tearDown() (ensure the method name TestOrganizationSlugLogo.tearDown is
updated to invoke super().tearDown() before/after any local cleanup) so the
BaseTest.tearDown logic runs in CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccbeb60b-acbc-459e-8ca8-68dfbc1ff90c
📒 Files selected for processing (1)
tests/test_organization_slug_logo.py
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tests/test_organization_slug_logo.py (3)
90-92:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winChain
BaseTest.tearDownto preserve CI delay.The
BaseTest.tearDown()includes a 1-second sleep when running in CI (GITHUB_ACTIONS == 'true'). Overriding without callingsuper().tearDown()skips that delay and can increase flakiness.🔧 Proposed fix
def tearDown(self): if self.org_id: self.scalekit_client.organization.delete_organization(organization_id=self.org_id) + super().tearDown()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_organization_slug_logo.py` around lines 90 - 92, The test's tearDown overrides BaseTest.tearDown and skips its CI delay, causing flakiness; update the tearDown in tests/test_organization_slug_logo.py to call super().tearDown() (i.e., preserve BaseTest.tearDown behavior which includes the 1s sleep when GITHUB_ACTIONS is true) before or after performing the organization deletion with scalekit_client.organization.delete_organization(organization_id=self.org_id) so the CI delay is not skipped.
39-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse unique slugs to avoid test collisions.
The hardcoded
slug = 'app.acmecorp.com'can cause failures if tests run in parallel or if cleanup from previous runs is incomplete.🔧 Proposed fix
- slug = 'app.acmecorp.com' + slug = f"app-{Faker().uuid4()[:8]}.acmecorp.com" organization = CreateOrganization( display_name=Faker().company(), external_id=Faker().uuid4(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_organization_slug_logo.py` around lines 39 - 51, The test_create_with_slug uses a hardcoded slug which can collide across runs; change the slug assignment to generate a unique value (e.g., append a UUID/timestamp or use Faker.unique) before building the CreateOrganization object so that CreateOrganization(..., slug=slug) and the subsequent assertion against response[0].organization.slug always use a unique slug when calling scalekit_client.organization.create_organization.
73-88:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse unique slugs to avoid test collisions.
The hardcoded
slug = 'app.acmecorp.com'can cause failures if tests run in parallel or if cleanup from previous runs is incomplete.🔧 Proposed fix
- slug = 'app.acmecorp.com' - metadata = {'custom_domain': 'app.acmecorp.com'} + slug = f"app-{Faker().uuid4()[:8]}.acmecorp.com" + metadata = {'custom_domain': slug} update_organization = UpdateOrganization(slug=slug, metadata=metadata)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_organization_slug_logo.py` around lines 73 - 88, The test_update_slug_and_metadata uses a hardcoded slug ('app.acmecorp.com') which can collide across parallel runs; update the test_update_slug_and_metadata to generate a unique slug per run (e.g., append a short uuid/timestamp or use the created org_id) before building the UpdateOrganization object so UpdateOrganization(slug=...) and metadata use that unique value; ensure assertions still compare against the generated slug and metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/test_organization_slug_logo.py`:
- Around line 90-92: The test's tearDown overrides BaseTest.tearDown and skips
its CI delay, causing flakiness; update the tearDown in
tests/test_organization_slug_logo.py to call super().tearDown() (i.e., preserve
BaseTest.tearDown behavior which includes the 1s sleep when GITHUB_ACTIONS is
true) before or after performing the organization deletion with
scalekit_client.organization.delete_organization(organization_id=self.org_id) so
the CI delay is not skipped.
- Around line 39-51: The test_create_with_slug uses a hardcoded slug which can
collide across runs; change the slug assignment to generate a unique value
(e.g., append a UUID/timestamp or use Faker.unique) before building the
CreateOrganization object so that CreateOrganization(..., slug=slug) and the
subsequent assertion against response[0].organization.slug always use a unique
slug when calling scalekit_client.organization.create_organization.
- Around line 73-88: The test_update_slug_and_metadata uses a hardcoded slug
('app.acmecorp.com') which can collide across parallel runs; update the
test_update_slug_and_metadata to generate a unique slug per run (e.g., append a
short uuid/timestamp or use the created org_id) before building the
UpdateOrganization object so UpdateOrganization(slug=...) and metadata use that
unique value; ensure assertions still compare against the generated slug and
metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 602c836f-6acd-4bec-8da1-10252d73fb9c
📒 Files selected for processing (1)
tests/test_organization_slug_logo.py
srinivaskarre-sk
changed the title
Removed feature-flag skip blocks that checked for logo_url support and simplified error handling. Added get_organization read-back assertions in test_update_logo_url and test_update_slug_and_metadata to verify persistence.
- Align test fixture with standardized test logo URL across SDKs
Replaces hardcoded 'auth.megasoft.com' slug with per-run UUID-based values to prevent collisions when tests are re-run. Chains super().tearDown() so BaseTest CI delay runs after each test for eventual-consistency safety.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MakefilewithPROTO_REF=v0.1.123.0and SSH-formatPROTO_REPO_URLcreate_organization(CreateOrganization(...))already supportslogo_urlandslugvia proto fieldsSummary by CodeRabbit
New Features
Tests