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

fix ignore conditions nil error #9560

Merged
merged 5 commits into from Apr 22, 2024
Merged

Conversation

jakecoffman
Copy link
Member

fixes #9457

This adds the stacktrace to the logs which allowed me to track down one of the other issues in #9457.

In job.json we started sending updated-at in the ignore conditions so it could be used for sorting, but here in the code it was used incorrectly as updated_at.

Then later when types were applied, a T.must was added which always fails because neither created_at nor created-at are ever sent in the payload.

It would be best if job.rb immediately parsed the job.json into types and passed those around instead of hashes. That would help avoid this problem in the future.

From what I can tell, updated-at should always be sent, but I was a bit defensive and set the time to the epoch for any that don't have that field set.

@jakecoffman
Copy link
Member Author

Looks like #7787 fixed the updated_at issue but then it got unfixed again.

@jakecoffman
Copy link
Member Author

The tests in #7787 were ineffective as it used the wrong key updated_at but also it was asserting the natural sort order. So once I fixed the key the test still passed.

By making business4 nil I'm able to test both the fallback logic and the key itself.

@jakecoffman jakecoffman marked this pull request as ready for review April 22, 2024 14:15
@jakecoffman jakecoffman requested a review from a team as a code owner April 22, 2024 14:15
@JamieMagee
Copy link
Contributor

I still don't have my permissions back, so I can't approve this PR, but LGTM :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Grouped update includes no details about upgrades
3 participants