feat: Add automatic tagging for github integration #4028
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Ignored Deployments
|
novakzaballa
changed the title
Uffizzi Ephemeral Environment
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4028 +/- ##
==========================================
+ Coverage 96.42% 96.44% +0.02%
==========================================
Files 1146 1150 +4
Lines 37386 37691 +305
==========================================
+ Hits 36048 36351 +303
- Misses 1338 1340 +2 ☔ View full report in Codecov by Sentry. |
novakzaballa
changed the title
api/conftest.py
Outdated
| from tests.unit.features.test_unit_feature_external_resources_views import ( | ||
| mocked_requests_post, | ||
| ) |
There was a problem hiding this comment.
I think mocked_requests_post should be converted into a fixture and added to a conftest.py instead of importing it like this.
There was a problem hiding this comment.
Converted into a fixture in conftest.py
api/conftest.py
Outdated
| ) | ||
| mock_generate_token.return_value = "mocked_token" | ||
|
|
||
| mocker.patch("requests.post", side_effect=mocked_requests_post) |
There was a problem hiding this comment.
Where is mocked_requests_post coming from?
There was a problem hiding this comment.
Converted into a fixture in conftest
| tag_by_event_type = { | ||
| "pull_request": { | ||
| "closed": GitHubTag.PR_CLOSED.value, | ||
| "converted_to_draft": GitHubTag.PR_DRAFT.value, | ||
| "opened": GitHubTag.PR_OPEN.value, | ||
| "reopened": GitHubTag.PR_OPEN.value, | ||
| "ready_for_review": GitHubTag.PR_OPEN.value, | ||
| "merged": GitHubTag.PR_MERGED.value, | ||
| }, | ||
| "issues": { | ||
| "closed": GitHubTag.ISSUE_CLOSED.value, | ||
| "opened": GitHubTag.ISSUE_OPEN.value, | ||
| "reopened": GitHubTag.ISSUE_OPEN.value, | ||
| }, | ||
| } |
There was a problem hiding this comment.
This section should probably live in the constants.py module.
There was a problem hiding this comment.
I was tempted to put it in constants, however, it would lose context and require extra comments since it's used only in the context of the github.py file. i.e. the event_type in its name refers to a webhook request event type.
api/integrations/github/github.py
Outdated
| def handle_installation_deleted(payload: dict[str, typing.Any]) -> None: | ||
| installation_id = payload.get("installation", {}).get("id") | ||
| if installation_id: |
There was a problem hiding this comment.
Do we really want to fail without an exception if installation_id is None?
There was a problem hiding this comment.
Good catch! At least initially we want an unhandled exception in case of these unexpected cases. GitHub should never send an "installation.delete" event without an installation ID, if that happens we want the alerts.
| if feature: | ||
| if ( |
There was a problem hiding this comment.
I would return early here.
if not feature:
return
if ( #....There was a problem hiding this comment.
In this case, it's the same, right? I will keep this since it is a positive condition, it is returning early anyway, and the code is readable in this way.
| from django.db import migrations | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): |
There was a problem hiding this comment.
You should be able to combine this migration with the other one.
There was a problem hiding this comment.
Right. Done.
| from integrations.github.constants import ( | ||
| GitHubTag, | ||
| github_tag_description, | ||
| ) |
There was a problem hiding this comment.
Are you sure you need to inline import these constants? Surely they would be topline imports since there should be no cyclical import issues.
There was a problem hiding this comment.
Yes. I have a circular import issue otherwise.
|
|
||
| print(feature_external_resource) |
There was a problem hiding this comment.
I think you want to remove this print statement.
| url = reverse("api-v1:github-webhook") | ||
|
|
||
| # When | ||
| client = APIClient() |
There was a problem hiding this comment.
Why not use the api_client fixture instead?
| # def test_handle_installation_deleted_with_non_existing_installation(mocker: MockerFixture): | ||
| # payload = {"installation": {"id": 456}} | ||
| # handle_installation_deleted(payload) | ||
| # mock_logger.error.assert_called_once_with( | ||
| # "Github Configuration with installation_id 456 does not exist" | ||
| # ) |
There was a problem hiding this comment.
Extra commented out code. Should be deleted, yes?
There was a problem hiding this comment.
Oops. Thanks! Deleted
novakzaballa
force-pushed
the
feat/add-tags-for-github-integration
branch
from
918207d
to
856e670
Compare
…ze comment titles Also, added `FLAG_UPDATED_FROM_GHA` event type.
Thanks for submitting a PR! Please check the boxes below:
pre-committo check lintingdocs/if required so people know about the feature!Changes
When a project is lilnked to a GitHub repository, the following tags are added to the project:
The tags added are:
PR OpenPR MergedPR ClosedPR DraftIssue OpenIssue ClosedAdd automatic tagging of features linked to GitHub resources. The tags are added on response to events in Flagsmith, like linking/unlinking PR/Issues, and also in response to events in GitHub like closing/reopening/marking as draft/merging PR/Issues. Each feature can have only one active PR tag and one active Issue tag at any time.
How did you test this code?
Unit tests added.
For local manual testing, the corresponding FE PR is #4035
It would be necessary to configure a local dev environment with these two branches (BE/FE) for testing until this BE PR is merged.
NOTE
For the case of multiple PRs/Issues linked to the same feature, we should consider tagging the feature as closed/merged only when all the linked Issues/PRs are closed/merged correspondingly.