[ENG-8558] update WB stuff that is explicit to resolve#465
Merged
felliott merged 8 commits intoCenterForOpenScience:feature/post-buff-wormsfrom Aug 14, 2025
Merged
Conversation
mkovalua
changed the title
cslzchen
approved these changes
Aug 13, 2025
Comment on lines
-133
to
+139
| # Todo: the commented solution may be more stable than not commented | ||
| # NOTE: These commented-out blocks are left as a reference to an | ||
| # alternative implementation. | ||
| # The current active implementation was chosen to maintain unification | ||
| # with the generic `make_request` pattern used all over providers (see PR #463 for discussion). | ||
| # A trade-off is that the current approach requires using `unquote` on the path to handle xml properly, | ||
| # which this paginator-based solution might avoid, however maybe there is more explicit solution without `unquote` | ||
| # for 'make_requests' to keep it more explicit. |
Contributor
There was a problem hiding this comment.
Nit-picking: language looks good but the format (line-breaks) looks rough, which will probably be done by the Ruff work soon, don't worry about it.
felliott
approved these changes
Aug 14, 2025
felliott
changed the base branch from
post-buff-worms
to
feature/post-buff-worms
* (Signature of method X does not match signature of base method Y of the base) CenterForOpenScience#463 (comment)
* method signature does not match overridden CenterForOpenScience#463 (comment)
* add extra note about alternative implementation of some methods for s3 provider CenterForOpenScience#463 (comment)
* S3 tests are skipped because request signing and client interaction have changed significantly on boto to boto3 migration, breaking the old test logic CenterForOpenScience#463 (comment)
* with new one test fails with the following error # FAILED tests/server/api/v1/test_core.py::TestBaseHandler::test_write_error # - TypeError: BaseHandler.write_error() takes 2 positional arguments but 3 were given maybe it cause additional issues (though old and new 'write_error' code looks to have same logic) even though we align test
* pytest_runtest_setup and pytest_runtest_teardown is not used on test suite run for now, it looks to be redundant CenterForOpenScience#463 (comment)
felliott
force-pushed
the
feature/ENG-8558
branch
from
60786b4 to
17773f3
Compare
felliott
merged commit 030b2a6
into
CenterForOpenScience:feature/post-buff-worms
1 of 2 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket
https://openscience.atlassian.net/browse/ENG-8558
Purpose
We merged the WB feature branch to MFR, letting a few minor issues pass. Let’s revisit the CR and address those. I will create a feature/post-buff-worms based on develop for you to target your PR.
Changes
Have gone through the ticket WB update ticket comments
#463
and updated stuff that is explicit to resolve,
for some non explicit stuff like some S3 possible post-release updates/investigation left comments
#463 (comment)
#463 (comment)
Side effects
QA Notes
Deployment Notes