Skip to content
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

Correctly calculate Job.started_at/Job.finished_at #655

Merged
merged 6 commits into from
May 16, 2023

Conversation

suricactus
Copy link
Collaborator

@suricactus suricactus commented May 15, 2023

Since before_docker_run/after_docker_run were called respenctively before/after setting the status and started_at/finished_at, it was causing timing issues in tests and not only.

This PR is aiming at fixing this.

TODO:

  • in PackageJobRun.after_docker_run() the job.status is used, untangle this [WIP]

@suricactus suricactus added bug Something isn't working patch Requires patch version change labels May 15, 2023
@suricactus suricactus requested a review from faebebin May 15, 2023 11:33
Since before_docker_run/after_docker_run were called respenctively before/after setting the status and started_at/finished_at, it was causing timing issues in tests and not only.
Copy link
Member

@faebebin faebebin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests fail - Should I take over fixing the tests? - is what I am doing on #650 too at the moment (in the waiting rooms ;)

@faebebin faebebin self-assigned this May 15, 2023
@suricactus
Copy link
Collaborator Author

suricactus commented May 15, 2023

If it's blocking - go ahead. The fix should be easy - the project.packaged_at is not saved anymore.

@faebebin faebebin removed their assignment May 15, 2023
Comment on lines 326 to 327
# only successfully finished packaging jobs should update the Project.data_last_packaged_at
if self.job.status == Job.Status.FINISHED:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check was never necessary, as before the job.status = Job.status.FINISHED was set right before calling after_docker_run .

for job in Job.objects.filter(
type=Job.Type.PACKAGE,
)
.exclude(id=self.job_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before self.job was excluded with FINISHED jobs. Now need to exclude seperately.

@faebebin faebebin merged commit e8280fb into master May 16, 2023
3 of 4 checks passed
@faebebin faebebin deleted the correct_job_status_on_time branch May 16, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working patch Requires patch version change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants