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

asu/update: avoid crashes on non-existent or obsolete versions #980

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

efahl
Copy link
Contributor

@efahl efahl commented Sep 19, 2024

Add some logic to gracefully ignore updates on old versions that lack the proper data to be supported.

Every time I did a cold start, I'd get a bunch of errors on the worker:

  File "/app/asu/update.py", line 26, in update_targets
    version_path = branch_data["path"].format(version=version)
                               ^^^^^^
KeyError: branch_data has no key "path"

Stuck an assertion in and turns out to be the 19.07 stuff,

AssertionError: No path for 19.07.7

Not sure if it happens in production, but this fixes it and cleans up a few little things.

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 72.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 89.28%. Comparing base (5e65dec) to head (d42b9a4).
Report is 99 commits behind head on main.

Files with missing lines Patch % Lines
asu/update.py 46.15% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #980      +/-   ##
==========================================
+ Coverage   80.75%   89.28%   +8.53%     
==========================================
  Files          15       14       -1     
  Lines         977     1027      +50     
==========================================
+ Hits          789      917     +128     
+ Misses        188      110      -78     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@efahl
Copy link
Contributor Author

efahl commented Sep 19, 2024

I tried for a few hours to write a test for this going through the api. Read this at least 10 times, https://fastapi.tiangolo.com/tutorial/testing/#extended-fastapi-app-file

I could never get the headers to pass through, the x_update_token value in the api update function was always None... Everything else got through, and token inside update was good, as expected.

def test_api_update_good(app):
    settings.update_token = "good"
    client = TestClient(app)
    headers = {"X-Update-Token:": settings.update_token}
    response = client.get("/api/v1/update/21.02.7/x86/64", headers=headers)
    assert response.status == 204

In asu/api.py:update, these are the values we have during the test run. Got any insight on how to get that header value passed in?

version = '21.02.7'
target = 'x86'
subtarget = '64'
response = <starlette.responses.Response object at 0x748b08442390>
x_update_token = None
settings.update_token = 'good'

@efahl
Copy link
Contributor Author

efahl commented Sep 19, 2024

Got it! Set the headers on the client and it works.

    settings.update_token = "good"
    client = TestClient(app)
    client.headers["X-Update-Token"] = settings.update_token

    response = client.get("/api/v1/update/21.02.7/x86/64")
    assert response.status_code == 204

Add some logic to gracefully ignore updates on old versions that lack the
proper data to be supported.

Signed-off-by: Eric Fahlgren <[email protected]>
@aparcar aparcar merged commit 18955f1 into openwrt:main Sep 20, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants