-
-
Notifications
You must be signed in to change notification settings - Fork 28
Fixing bytea json conversion error (update to hibernate 6, spring 3) #692
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
base: main
Are you sure you want to change the base?
Conversation
8e77916
to
1740b90
Compare
1740b90
to
69c3b4d
Compare
69c3b4d
to
724d28f
Compare
Update tests and remove code warnings checking for null.
724d28f
to
b4029d0
Compare
I think I got all the changes out, and merged. Those were mainly deprecation warnings that I found while trying to run the code, and a missing header. Now wait for GH Actions, then I will perform one final test. Fingers-crossed it will pass, and then it will be ready for someone to review. 🤞 |
It worked! To review, one can confirm the code looks alright, GH Actions build is passing, and maybe try @martinchapman I asked @mr-c to review it, but it would be great if you could try it too, if you have time (or maybe after this gets merged too), to confirm the issue is gone and the viewer is now working for you. Cheers |
Thank you very much @kinow ! Do you know why this wasn't caught by the CI? Do we need a new test? |
Not a problem, @mr-c 🙂 Good point! I think we didn't have a test for retrieving a workflow (or we did, but not necessarily for the part of the code where this happened), only for the queuedworkflow. I'll write a test once I have time to work on this again. That should give @chapmanb time to take a look too. Thanks! |
|
||
query.setParameter("retrievedFrom", retrievedFrom, new JsonType(GitDetails.class)); | ||
return (Workflow) query.uniqueResult(); | ||
} |
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.
Note to self: write a test for this!
Fixed last warning (it was complaining about our old version of postgres -- 9, while the latest is 17), and added a unit test. Note, there was one test consistently failing for me, due to a timeout to retrieve spdx licenses. I wonder whether it's a IP block, or maybe only GH Action/AWS/etc are allowed. But if that test fails, it can be ignored as it's not caused by any of the recent changes (you can try |
bb68f06
to
9af3d26
Compare
9af3d26
to
791e4e0
Compare
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at |
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.
cwlviewer is not part of Apache, so perhaps we shouldn't say this?
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.
+1. Huh, I think I copied the header from another file in the project. I hope I didn't accidentally copy-paste from a file outside the project (or a file that was copied from an AL project).
Description
Closes #664
WIP: will fill in rest later, and move commits to separate pull requests. Just removing warnings while I investigate the issue in #664.
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: