feat(perforce): Add backend support for Perforce integration #103171
Conversation
github-actions
bot
added
the
Scope: Backend
|
This PR is 2500 lines -- would it possible to split it up for review? There are aspects that should be reviewed by different teams like dependencies, specific features, etc |
mujacica
force-pushed
the
feat/perforce-backend
branch
from
f566dcc to
ab41785
Compare
mujacica
force-pushed
the
feat/perforce-backend
branch
2 times, most recently
from
2172a79 to
09e3105
Compare
mujacica
force-pushed
the
feat/perforce-backend
branch
from
e293bd2 to
2a52e60
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #103171 +/- ##
===========================================
- Coverage 80.59% 80.54% -0.06%
===========================================
Files 9297 9297
Lines 396850 397078 +228
Branches 25281 25281
===========================================
- Hits 319839 319809 -30
- Misses 76551 76809 +258
Partials 460 460 |
mujacica
force-pushed
the
feat/perforce-integration
branch
from
a978a06 to
1197dcd
Compare
mujacica
force-pushed
the
feat/perforce-backend
branch
from
2a52e60 to
ecfa2ce
Compare
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Christinarlong
left a comment
Christinarlong
left a comment
There was a problem hiding this comment.
Could we break this PR down more to be on the feature level? This PR is still pretty insane to review and push out at once since there's like 4-5 features here. Maybe something like
- integration installation & authentication ?
- stacktrace linking
- repository management
- releases/commit tracking
- suspect commits
mujacica
force-pushed
the
feat/perforce-backend
branch
from
4cce358 to
eea34fb
Compare
cathteng
left a comment
cathteng
left a comment
There was a problem hiding this comment.
Generally looks good, I haven't looked at the tests too thoroughly because I'm not familiar with how Perforce constructs its URLs 😅
| self.P4 = P4 | ||
| self.P4Exception = P4Exception |
There was a problem hiding this comment.
nit: does this need to be in self if it's always the same thing?
| path_without_rev, revision = path.rsplit("#", 1) | ||
|
|
||
| # If already absolute depot path, use as-is | ||
| if path_without_rev.startswith("//"): |
There was a problem hiding this comment.
Is it guaranteed that anything that starts with // will point to the absolute path?
| ) | ||
|
|
||
| # Apply pagination limit if specified | ||
| if page_number_limit and len(repositories) >= page_number_limit: |
There was a problem hiding this comment.
How does this apply the page number limit? For GitHub this is the max number of pages we will process because the API has pagination, I'm not sure it's the same thing as the total number of repos.
The page_number_limit is in the function signature because GH uses it, there are some source code integrations that don't use it so I think you can consider it optional
| """ | ||
| return self.model.metadata | ||
|
|
||
| def update_organization_config(self, data: Mapping[str, Any]) -> None: |
There was a problem hiding this comment.
I'm unsure if this function is necessary because we don't surface anywhere for you to be able to update the integration details after you install the integration (besides if you hit the OrganizationIntegrationDetails endpoint directly) and we don't actually store information on the OrganizationIntegration
Like Gitlab, you could leave get_organization_config and update_organization_config unimplemented in the PerforceIntegration class (it will return [] and be fine)
| external_id = f"perforce-org-{organization_id}-{p4port_hash}" | ||
|
|
||
| # Store credentials in Integration.metadata | ||
| metadata = { |
| pipeline.bind_state("p4port", form_data.get("p4port")) | ||
| pipeline.bind_state("name", f"Perforce ({form_data.get('p4port')})") | ||
| # Include organization_id to create unique external_id per org | ||
| pipeline.bind_state("organization_id", pipeline.organization.id) |
There was a problem hiding this comment.
You can also use determine_active_organization(request) when you need to pull the organization in the next step
| pipeline.bind_state("p4port", form_data.get("p4port")) | ||
| pipeline.bind_state("name", f"Perforce ({form_data.get('p4port')})") |
There was a problem hiding this comment.
nit: do you need to bind these if they're already in the form data?
This commit adds backend support for Perforce version control integration:
Requires: #103287