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

Update pool url on redirect #17384

Merged

Conversation

felixbrucker
Copy link
Contributor

Purpose:

Support the migration of pool farmer api urls: when a pool changes its farmer api url, the client persists the new url to be able to continue to communicate with the pool should the old url stop working in the future.

Current Behavior:

Pools can already setup redirects to a new farmer api url and they will get used correctly, however the new url is not persisted by the client.

New Behavior:

When a url change is detected the new url is persisted.

@felixbrucker felixbrucker requested a review from a team as a code owner January 23, 2024 15:56
@felixbrucker felixbrucker force-pushed the update-pool-url-from-redirect branch from 5a61b57 to 4952d73 Compare January 23, 2024 16:18
@emlowe emlowe added the Added Required label for PR that categorizes merge commit message as "Added" for changelog label Jan 23, 2024
@emlowe
Copy link
Contributor

emlowe commented Jan 23, 2024

Needs a test to execise this code - take a look at test_farmer_pool_response

@felixbrucker
Copy link
Contributor Author

felixbrucker commented Jan 24, 2024

Unfortunately i know too little of how the mocking in pytest works for the following scenario:
- mock GET for one url with a 307 redirect and location set to another url
- mock a second GET for the other url with a json response

It seems the currently used patch just overwrites what the specific aiohttp.ClientSession.* method returns, i'd need something like https://github.com/pnuckowski/aioresponses probably, or is something like this achievable with some kind of callback style return value where i do the matching myself?

Edit: or i just skip all the redirect logic in aiohttp and return the final response with the url being different from the requested url, thinking about that now it's probably the better way anyway as how redirects are handled by aiohttp is not in scope here. I'll implement a test using this later 👍

@felixbrucker
Copy link
Contributor Author

@emlowe added tests 👍

chia/farmer/farmer.py Outdated Show resolved Hide resolved
@felixbrucker felixbrucker requested a review from arvidn January 25, 2024 12:42
@felixbrucker felixbrucker requested a review from emlowe January 30, 2024 03:58
@emlowe
Copy link
Contributor

emlowe commented Jan 30, 2024

close and reopen to pickup hopefully some CI fixes

@emlowe emlowe closed this Jan 30, 2024
@emlowe emlowe reopened this Jan 30, 2024
@emlowe
Copy link
Contributor

emlowe commented Jan 31, 2024

coverage diff exemption

@Starttoaster Starttoaster merged commit 728f956 into Chia-Network:main Jan 31, 2024
501 of 514 checks passed
@felixbrucker felixbrucker deleted the update-pool-url-from-redirect branch January 31, 2024 17:54
emlowe added a commit that referenced this pull request Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added Required label for PR that categorizes merge commit message as "Added" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants