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: API returns invalid file URLs, due to including the port #2223
Conversation
Codecov ReportBase: 86.64% // Head: 86.87% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2223 +/- ##
==========================================
+ Coverage 86.64% 86.87% +0.23%
==========================================
Files 233 241 +8
Lines 28020 27994 -26
==========================================
+ Hits 24277 24320 +43
+ Misses 3743 3674 -69
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
Approved, but check my suggestion/section.
val internalBaseUrl: String = | ||
internalProtocol + "://" + internalHost + (if (internalPort != 80) ":" + internalPort else "") | ||
val externalBaseUrl: String = | ||
externalProtocol + "://" + externalHost + (if (externalPort != 80) ":" + externalPort else "") |
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.
val internalBaseUrl: String = | |
internalProtocol + "://" + internalHost + (if (internalPort != 80) ":" + internalPort else "") | |
val externalBaseUrl: String = | |
externalProtocol + "://" + externalHost + (if (externalPort != 80) ":" + externalPort else "") | |
val internalBaseUrl: String = internalProtocol + "://" + internalHost | |
val externalBaseUrl: String = externalProtocol + "://" + externalHost |
Is the port needed here at all? If not then it should be shortened as above.
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.
hard to say, it might break locally, or in tests... But even if so, I do suspect that externalPort != 80
does not make sense, it should be 1024
if anything, because that's the config default, which should only happen when there is no environment variable set (and it's also what the docker-compose specifies, when ewe run it locally).
But I am reluctant to change these things now: Then we might not know what actually fixed the but. So IMO it would be best to only change what we expect to be the core issue, and then do a separate cleanup of all this stuff in a dedicated task. The same should also be done with all other configs, not only SIPI - but then we'll have to do much more testing, so we can't deploy the fix to prod anytime soon... Does that make sense?
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.
That means you are guessing a lot, but if you feel there is no other way to make sure what and if it works, then go ahead.
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.
yes... it's a guessing fix at that point (though there is a good chance this will actually fix it, we did do some testing). But the guessing thing only gets harder when we change more things, so I'd actually prefer it like this, I think.
Issue Number: DEV-1378
Pull Request Checklist
Basic Requirements
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Does this PR change client-test-data?
Other information