feat: use configured deploy job for deployment time estimates#939
feat: use configured deploy job for deployment time estimates#939mertilginoglu wants to merge 4 commits intostagingfrom
Conversation
3395ab1 to
3f5aa10
Compare
Not up to standards ⛔🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity |
🟢 Coverage 45.34% diff coverage · +0.14% coverage variation
Metric Results Coverage variation ✅ +0.14% coverage variation (-1.00%) Diff coverage ✅ 45.34% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (0518e03) 8956 3269 36.50% Head commit (a0573dd) 9112 (+156) 3339 (+70) 36.64% (+0.14%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#939) 161 73 45.34% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0573ddc2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return deploymentWorkflowConfigService | ||
| .findByWorkflowId(workflowId) | ||
| .map(ResponseEntity::ok) |
There was a problem hiding this comment.
Enforce repository ownership on deployment-config reads
Validate repositoryId before returning a workflow config. This endpoint currently ignores the path repository and fetches by workflowId alone, so a caller who can access repository A can request /api/settings/{A}/workflows/{workflowId-from-B}/deployment-config and receive repository B’s configuration if they know/guess the workflow ID. This is an authorization boundary bypass and should be fixed by checking the workflow belongs to repositoryId (as done in upsert).
Useful? React with 👍 / 👎.
| * @return A wrapper object containing the latest Deployment or HeliosDeployment, or an empty | ||
| * result if no deployments exist. | ||
| */ | ||
| private DeploymentDurationEstimate computeEstimate(Environment environment) { |
There was a problem hiding this comment.
It could be better if we replace this computeEstimate method after findLatestDeployment method because now it has wrong JavaDoc
| // HOTFIX: Flakiness recomputation is temporarily disabled to avoid OOMs in production. | ||
| // TODO: Re-enable after memory-safe flakiness path is deployed. | ||
| log.warn("HOTFIX active: skipping flakiness update for workflow run {}", workflowRun.getName()); | ||
| log.warn( |
There was a problem hiding this comment.
I enabled this flakiness scores update method in staging, which causes merge-conflict now.
meryemefe
left a comment
There was a problem hiding this comment.
I tested locally. It works very well. I have only some minor comments. Also, I would like to remind that you need to update the version of flyway while merging conflicts.
Motivation
Helios currently uses fixed timing assumptions when estimating how long a deployment will stay in pending and in-progress states. That makes the deployment progress view inaccurate for workflows whose actual deploy step starts much later or earlier than the default assumption. This change lets repositories configure the GitHub Actions job that represents the real deployment start so Helios can calculate more accurate build and deploy duration estimates.
Description
deployJobNameper workflowTesting Instructions
Prerequisites:
Flow:
SettingsConfigurein the newDeployment Jobcolumn for a deployment workflowChecklist
General
Server