-
Notifications
You must be signed in to change notification settings - Fork 187
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
Add S3/MinIO support for application settings #2733
Conversation
src/main/java/org/prebid/server/settings/S3ApplicationSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/settings/S3ApplicationSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/settings/S3ApplicationSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/settings/S3ApplicationSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/settings/S3ApplicationSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/settings/S3ApplicationSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/settings/service/S3PeriodicRefreshService.java
Outdated
Show resolved
Hide resolved
@And1sS I hope I resolved all the issues. Do you mind taking a look 🧐 |
src/main/java/org/prebid/server/settings/service/S3PeriodicRefreshService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/settings/service/S3PeriodicRefreshService.java
Outdated
Show resolved
Hide resolved
@muuki88 Java Ci failed, due to checkstyle |
Java CI failed because of checkstyle
|
Again, checkstyle :) Usually, to avoid this, before pushing, we run the command
|
Some tests inside Java CI failed
|
The tests fail due to the requested
|
Yeah, you are right. Thanks that you accidentally found it :) Bug-fix: #3385 Regarding your question about "how to mock vertx.getOrCreateContext() call": see |
I did it a little bit differently to keep the assertions being made |
src/main/java/org/prebid/server/spring/config/SettingsConfiguration.java
Outdated
Show resolved
Hide resolved
Sorry for my confusion. Should I use I guess this is the only thing left. And the adjustments to tests, depending on the decision. |
Better to use |
@muuki88, Thank you so much for your hard work and contributions on PR #2733. Your efforts laid a strong foundation, and we genuinely appreciate the time and energy you invested in this. Since we needed to move forward and complete the work, we created a new PR (PR #3418) based on your original changes. The new PR is now finished, and we’re just waiting for your approval to proceed. We’re truly grateful for your contributions, and your work was instrumental in getting us to this point. We would greatly appreciate it if you could take a moment to review and approve the new PR when you have a chance. Thanks again! |
That means a lot ❤️ thank you. I'll try to review it as soon as possible. Currently I'm on vacation |
Duplication of #3418 |
This PR adds support for an additional storage system to retrieve
Any S3 compatible system can be used.
Usage
It work's bascially the same as the file system settings, but the files are on S3. What we found, works really well, is using the ad unit path of GAM for the stored impression ID as this maps nicely to paths.
Credits
Credits goe to @0xlee for the intitial idea and @rmattis for the actual implementation.