Merged
Conversation
|
Thank you for submitting this pull request! We appreciate your contribution to the project. Before we can merge it, we need to review the changes you've made to ensure they align with our code standards and meet the requirements of the project. We'll get back to you as soon as we can with feedback. Thanks again! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's GuideExtends the preview app deployment workflow to react to pull request openings/reopenings, adds a notification job that comments/upserts preview environment status on the PR, and strengthens cleanup by using a PAT (if available) and deleting the corresponding deployment when the preview is removed. Sequence diagram for PR open/reopen preview status comment workflowsequenceDiagram
actor Developer
participant GitHub
participant DeployPreviewWorkflow
participant NotifyPROnOpenJob
participant GitHubAPI
participant PullRequest
Developer->>GitHub: Open_or_reopen_PR(branch)
GitHub-->>DeployPreviewWorkflow: pull_request_event(opened_or_reopened)
DeployPreviewWorkflow-->>NotifyPROnOpenJob: Start_job_if_branch_not_main_or_next
NotifyPROnOpenJob->>NotifyPROnOpenJob: Compute_BRANCH_SLUG
NotifyPROnOpenJob->>NotifyPROnOpenJob: Set_KAMAL_APP_NAME_and_KAMAL_APP_DOMAIN
NotifyPROnOpenJob->>GitHubAPI: listDeployments(environment=KAMAL_APP_NAME, per_page=1)
GitHubAPI-->>NotifyPROnOpenJob: deployments
alt Deployment_exists
NotifyPROnOpenJob->>GitHubAPI: listDeploymentStatuses(deployment_id, per_page=1)
GitHubAPI-->>NotifyPROnOpenJob: statuses
alt state_success
NotifyPROnOpenJob->>NotifyPROnOpenJob: Build_body_preview_ready_with_URL_and_timestamp
else state_in_progress
NotifyPROnOpenJob->>NotifyPROnOpenJob: Build_body_preview_in_progress
else other_state
NotifyPROnOpenJob->>NotifyPROnOpenJob: Build_body_preview_after_next_push
end
else No_deployments
NotifyPROnOpenJob->>NotifyPROnOpenJob: Build_body_preview_after_next_push
end
NotifyPROnOpenJob->>GitHubAPI: listComments(issue_number)
GitHubAPI-->>NotifyPROnOpenJob: comments
alt Existing_preview_comment_found
NotifyPROnOpenJob->>GitHubAPI: updateComment(comment_id, body)
else No_existing_preview_comment
NotifyPROnOpenJob->>GitHubAPI: createComment(issue_number, body)
end
GitHubAPI-->>PullRequest: Updated_or_new_preview_status_comment
Sequence diagram for PR close preview cleanup and deployment deletionsequenceDiagram
actor Developer
participant GitHub
participant DeployPreviewWorkflow
participant RemovePreviewJob
participant GitHubAPI
participant PreviewDeployment
Developer->>GitHub: Close_PR
GitHub-->>DeployPreviewWorkflow: pull_request_event(closed)
DeployPreviewWorkflow-->>RemovePreviewJob: Start_remove_preview_job
RemovePreviewJob->>GitHubAPI: listDeployments(environment=KAMAL_APP_NAME)
GitHubAPI-->>RemovePreviewJob: deployments
loop For_each_deployment
RemovePreviewJob->>GitHubAPI: createDeploymentStatus(deployment_id, state=inactive, description=Preview_environment_removed)
GitHubAPI-->>PreviewDeployment: Mark_inactive
RemovePreviewJob->>GitHubAPI: deleteDeployment(deployment_id)
GitHubAPI-->>RemovePreviewJob: Deployment_deleted
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Preview deployment readyPreview URL: https://website-next-remove.fairdataihub.org
|
|
Thanks for making updates to your pull request. Our team will take a look and provide feedback as soon as possible. Please wait for any GitHub Actions to complete before editing your pull request. If you have any additional questions or concerns, feel free to let us know. Thank you for your contributions! |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The branch-to-
KAMAL_APP_NAME/KAMAL_APP_DOMAINslug generation logic is duplicated in the newnotify-pr-on-openjob; consider extracting this into a single shared step or composite action to avoid future drift between jobs. - The logic for finding an existing preview comment relies on
c.user.type === 'Bot', which will miss comments created via a PAT or non-bot user; using a more robust marker (e.g., a hidden HTML comment or a stricter body prefix) would make comment updates more reliable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The branch-to-`KAMAL_APP_NAME`/`KAMAL_APP_DOMAIN` slug generation logic is duplicated in the new `notify-pr-on-open` job; consider extracting this into a single shared step or composite action to avoid future drift between jobs.
- The logic for finding an existing preview comment relies on `c.user.type === 'Bot'`, which will miss comments created via a PAT or non-bot user; using a more robust marker (e.g., a hidden HTML comment or a stricter body prefix) would make comment updates more reliable.
## Individual Comments
### Comment 1
<location path=".github/workflows/deploy-preview-app.yml" line_range="296-297" />
<code_context>
+ issue_number,
+ });
+
+ const existing = comments.find(
+ c => c.user.type === 'Bot' && c.body.startsWith('## Preview deployment')
+ );
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Comment-matching logic may fail when using a PAT, causing duplicate preview comments.
The `c.user.type === 'Bot'` check only matches GitHub App bots (e.g. `github-actions[bot]`). With `GH_PAT`, `c.user.type` will be `'User'`, so existing preview comments won’t be found and a new one will be created on each PR open/reopen. Relax this filter (e.g. rely on `body.startsWith('## Preview deployment')` and/or check `c.user.login` against the expected account) so the workflow updates the existing comment instead of duplicating it.
</issue_to_address>
### Comment 2
<location path=".github/workflows/deploy-preview-app.yml" line_range="414-418" />
<code_context>
state: 'inactive',
description: 'Preview environment removed'
});
+ await github.rest.repos.deleteDeployment({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ deployment_id: deployment.id
+ });
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Uncaught errors from deleteDeployment can fail the cleanup job unnecessarily.
This call isn’t guarded like the later cleanup steps. If it throws (e.g., 404, permission error, transient API issue), the job will fail even though the deployment is already marked inactive. Please wrap this in try/catch or handle expected statuses (like 404/409) so best-effort cleanup doesn’t cause the overall workflow to fail.
```suggestion
try {
await github.rest.repos.deleteDeployment({
owner: context.repo.owner,
repo: context.repo.repo,
deployment_id: deployment.id
});
} catch (error) {
if (error && (error.status === 404 || error.status === 409)) {
// Deployment already gone or in a conflicting state – ignore
core?.info
? core.info(`Non-fatal error deleting deployment ${deployment.id}: ${error.status}`)
: console.warn(`Non-fatal error deleting deployment ${deployment.id}:`, error);
} else {
// Best-effort cleanup: log but do not fail the job
core?.warning
? core.warning(`Failed to delete deployment ${deployment.id}: ${error.message || error}`)
: console.warn(`Failed to delete deployment ${deployment.id}:`, error);
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+414
to
+418
| await github.rest.repos.deleteDeployment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| deployment_id: deployment.id | ||
| }); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Uncaught errors from deleteDeployment can fail the cleanup job unnecessarily.
This call isn’t guarded like the later cleanup steps. If it throws (e.g., 404, permission error, transient API issue), the job will fail even though the deployment is already marked inactive. Please wrap this in try/catch or handle expected statuses (like 404/409) so best-effort cleanup doesn’t cause the overall workflow to fail.
Suggested change
| await github.rest.repos.deleteDeployment({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| deployment_id: deployment.id | |
| }); | |
| try { | |
| await github.rest.repos.deleteDeployment({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| deployment_id: deployment.id | |
| }); | |
| } catch (error) { | |
| if (error && (error.status === 404 || error.status === 409)) { | |
| // Deployment already gone or in a conflicting state – ignore | |
| core?.info | |
| ? core.info(`Non-fatal error deleting deployment ${deployment.id}: ${error.status}`) | |
| : console.warn(`Non-fatal error deleting deployment ${deployment.id}:`, error); | |
| } else { | |
| // Best-effort cleanup: log but do not fail the job | |
| core?.warning | |
| ? core.warning(`Failed to delete deployment ${deployment.id}: ${error.message || error}`) | |
| : console.warn(`Failed to delete deployment ${deployment.id}:`, error); | |
| } | |
| } |
|
Thanks for closing this pull request! If you have any further questions, please feel free to open a new issue. We are always happy to help! |
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 by Sourcery
Extend preview deployment workflow to notify pull requests about preview environment status on open and ensure deployments are fully cleaned up on removal.
CI: