-
Notifications
You must be signed in to change notification settings - Fork 531
#11546 fix compare separator logic #11596
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
Conversation
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
pdurbin
left a comment
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.
In the interest of keeping things moving I'm approving this. I didn't test it but the code looks ok to me. Also, API tests are passing.
@sekmiller did you check with @qqmyers about this fix? When he briefly went over #11546 during Triage Tuesday the other day, I was under the impression that he expected we'd need to add a Flyway script. I clicked "request review" from him, at least. 🤷
|
The Flyway script would have been a bandaid and would not help in the case where files are created while file PIDs are not enabled, and an attempt was made to publish after file PIDs were enabled |
|
fix looks good. Merging. |
… fix compare separator logic
What this PR does / why we need it: Publishing was failing when separator was empty and didn't match the default separator
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this: create files with file PIDs disabled then enable file PIDs (curl -X PUT -d 'true' http://localhost:8080/api/admin/settings/:FilePIDsEnabled) you should be able to publish and get file PIDs assigned.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: