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

Merge in updated libraries from Cantaloup 5.0.6 and fix test failures #656

Open
wants to merge 54 commits into
base: develop
Choose a base branch
from

Conversation

angelahuqing
Copy link

@angelahuqing angelahuqing commented May 9, 2024

5.0.6 is merged into develop branch as well as replacing or removing tests/functions that aren't being used and causing failure.

Alex Dolski and others added 30 commits March 25, 2022 14:16
@glenrobson
Copy link

glenrobson commented May 22, 2024

Hi @angelahuqing, thank you for submitting this pull request and doing all of the work to bring in the changes. Are you able to make the Cantaloupe meeting later?

I've had a look at the code and there are a few changes which seem wider than just the library version update. Specificlly:

  • What is the affect of removing UriCompliance?
    25fa822
  • I can see you've removed XMPElements and mention that it isn't used but I can see its used in the delegates script which means some people might be relying on it.
  • I can see you've also removed the s3 multipart output stream and related tests. Is this no longer needed?

I'm happy to close my pull request (#653) in favour of this one but just want to check we aren't removing needed functionality. Thanks

@angelahuqing
Copy link
Author

Hi @glenrobson I think I may have missed the meeting, but if you let me know when the next one is, I can try and attend. To give you some more context on the changes I made, all of the changes came as a result of failing tests in the GA. Anything that I removed was because it was a direct cause of a failing test. I also cross-checked with the 5.0.6 release to see what hadn't been automatically merged and deduced from that what could be removed. For example

  • the multipart output stream is no longer being used in 5.0.6 and was causing test failures which is why I removed it and replaced it with the function that is being used in 5.0.6 in the same instances. 5.0.6 and dev are now more in sync with one another because of it.
  • UriConpliance was also failing tests and isn't in 5.0.6 anymore and didn't seem necessary in dev
  • I didn't realize the XMPElements are being used in the delegate scripts. The XMPElements methods that I removed aren't in 5.0.6.

@glenrobson
Copy link

Ah I can see that the XMP Elements were added here to the develop branch. I can't see a related issue requesting this feature but useful to know its something new.

Also the multipart upload for s3 was added here and we discussed in the meeting that this would help with large images that are cached.

Is there a way we could look at fixing the breaking tests for XMP and s3 without removing them?

@angelahuqing
Copy link
Author

I think it's possible to fix the tests for XMP and s3. I didn't look into it too much because this PR was focused on getting the merge between dev and 5.0.6 done without failures so that the security issues would be fixed. I don't necessarily know if I will have the time to debug them, but I think someone theoretically could.

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.

None yet

2 participants