-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: UI Jobs streams are not cleaned up after PR is closed #5359
base: main
Are you sure you want to change the base?
Conversation
Update `func (p *PullClosedExecutor) CleanUpPull` to use the updated definition of `PullInfo` Signed-off-by: joe1981al <[email protected]> Signed-off-by: Joseph McDonald <[email protected]>
Signed-off-by: Joseph McDonald <[email protected]>
PullNum: pull.Num, | ||
Repo: pull.BaseRepo.Name, | ||
RepoFullName: pull.BaseRepo.FullName, | ||
ProjectName: project.ProjectName, | ||
Path: project.RepoRelDir, | ||
Workspace: project.Workspace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also add unit tests for such changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chenrui333, I'm not deeply familiar with go unit testing, but it looks like there are test around this, but problem was not enough information was being passed to match what was actually in the map, so it thought everything related was being cleaned up but it was not matching the actual entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More details, on debugging done on p.LogStreamResourceCleaner.CleanUp(jobContext) that this calls.
I added more logging to see what the contents of the map p.pullToJobMapping
, and this is what I had (removing identifying info):
{
"{54106 repo azureorg/project/repo terraform/project/path default}": {
"0cea6de8-5618-405f-b819-42d377b6f5ae": {
"JobID": "0cea6de8-5618-405f-b819-42d377b6f5ae",
"JobIDUrl": "",
"JobDescription": "",
"Time": "2025-02-21T12:44:55.947183432-06:00",
"TimeFormatted": "",
"JobStep": "plan"
}
}
}
but looking at the input to the function:
jobs.PullInfo {PullNum: 54106, Repo: "repo", RepoFullName: "", ProjectName: "", Path: "", Workspace: "default"}
Some of the objects in the key were not included in the input, so the if statement for the matching was just skipping the removal since no maps matched and there are no checks to make sure items related to a pull are removed from the mapping.
what
func (p *PullClosedExecutor) CleanUpPull
to use the updated definition of `PullInfowhy
PullInfo
PullInfo
wasn't updated in function used to cleanup job info, so matches were not found in the mappingtests
references