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

feat(web): Add web server command to serve JSON resume from URL with periodic refresh #241

Merged
merged 4 commits into from
Nov 18, 2024

Conversation

kiraum
Copy link
Contributor

@kiraum kiraum commented Oct 26, 2024

This PR introduces a new web command for serve allowing to serving an JSON resume from a URL with periodic refresh. Key features include:

  • New web command in __main__.py with options for URL, refresh interval, port, and host;
  • Implementation of WebHandler class in server.py to handle fetching, rendering, and serving the resume;
  • Periodic refresh of resume data from the specified URL;
  • Caching mechanism to serve the last valid render if fetching fails.

This feature enables users to serve their resume from external sources, making it easier to keep the displayed resume up-to-date without manual intervention.

Notes:

  • I did run all tests manually required on devbox, but I don't use devbox/nix (I try to install as less software as possible on my laptop);
    -- On my setup, I normally include optional-dependencies on pyproject, with that I can use uv pip compile --extra dev > requirements-dev.txt, and have a local pre-commit.
  • Built the image and seems to be working as expected, it's on production here: curl https://ansiv.xpto.it;
  • Very nice change moving from poetry to uv;

Please, let me know if something doesn't look good, and/or if I missed something, and feel free to reject/close the PR if it's not something aligned with the project.

Thanks for the very nice project/repo/code, learning a lot with it! =D

Copy link
Owner

@alexpovel alexpovel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR! Looks nice! 🙂

A couple notes:

  • devbox is totally optional, so if you can replicate it locally more or less, that's totally fine. CI uses devbox though, and that ultimately needs to pass. That would be the strongest argument in favor of devbox. That said, I've been thinking of ditching devbox too, because...
  • ...I only used it for poetry, which sure enough broke down immediately when I tried to get ancv running locally again. uv seems much more robust by itself!
  • does [tool.uv] dev-dependencies in pyproject.toml work for you? It has all dev dependencies there. I'm new to uv so happy to change that if there's a better way

CI is currently failing, I think because of GitHub-specific API token problems... so never mind that for now.

Two general notes:

  • you can add your new functionality to docs, e.g. the Selfhosting section of the README.
  • a test will be needed I'm afraid! It should check for basic functionality. You you try and set the destination to a local server like https://docs.python.org/3/library/http.server.html , which is spun up for the test (similar to https://pkg.go.dev/net/http/httptest#NewServer if you're more familiar with Go). Checking the caching functionality would also be nice, you could set refresh to a second and play with that

ancv/web/server.py Outdated Show resolved Hide resolved
ancv/web/server.py Outdated Show resolved Hide resolved
ancv/web/server.py Outdated Show resolved Hide resolved
ancv/web/server.py Outdated Show resolved Hide resolved
ancv/web/server.py Outdated Show resolved Hide resolved
ancv/web/server.py Outdated Show resolved Hide resolved
ancv/web/server.py Outdated Show resolved Hide resolved
ancv/web/server.py Outdated Show resolved Hide resolved
ancv/web/server.py Show resolved Hide resolved
ancv/web/server.py Outdated Show resolved Hide resolved
@kiraum
Copy link
Contributor Author

kiraum commented Oct 27, 2024

Thanks for your PR! Looks nice! 🙂

A couple notes:

* devbox is totally optional, so if you can replicate it locally more or less, that's totally fine. CI uses devbox though, and that ultimately needs to pass. That would be the strongest argument in favor of devbox. That said, I've been thinking of ditching devbox too, because...

* ...I only used it for `poetry`, which sure enough broke down immediately when I tried to get ancv running locally again. `uv` seems much more robust by itself!

* does `[tool.uv] dev-dependencies` in `pyproject.toml` work for you? It has all dev dependencies there. I'm new to `uv` so happy to change that if there's a better way

CI is currently failing, I think because of GitHub-specific API token problems... so never mind that for now.

Two general notes:

* you can add your new functionality to docs, e.g. the _Selfhosting_ section of the README.

* a test will be needed I'm afraid! It should check for basic functionality. You you try and set the `destination` to a local server like https://docs.python.org/3/library/http.server.html , which is spun up for the test (similar to https://pkg.go.dev/net/http/httptest#NewServer if you're more familiar with Go). Checking the caching functionality would also be nice, you could set `refresh` to a second and play with that

Thanks for the quick answer/comments, I will go over it during the week, a bit slow here, because it's herfstvakantie (autumn holidays), so a bit busy with family! 😅

I never tested [tool.uv] dev-dependencies, but I will take a look, I'm also new to uv and ruff, but really happy with it!

I normally use something like this:

» git diff pyproject.toml
diff --git a/pyproject.toml b/pyproject.toml
index 7ded164..9af67d5 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -41,6 +41,13 @@ dependencies = [
     "pydantic[email]>=2.9.2",
 ]

+[project.optional-dependencies]
+dev = [
+    "black>=23.0",
+    "mypy>=1.0",
+    "ruff>=0.0.290",
+]
+
 [project.urls]
 Homepage = "https://ancv.io"
 Repository = "https://github.com/alexpovel/ancv/"

With that, I can compile the requirements and requiments only for dev env:

» uv pip compile pyproject.toml > requirements.txt
Resolved 32 packages in 284ms
» uv pip compile pyproject.toml --extra dev > requirements-dev.txt
Resolved 39 packages in 191ms

Check the final diff:

» diff -U0 requirements.txt requirements-dev.txt | grep -vE '^@|#'
--- requirements.txt    2024-10-27 18:47:56
+++ requirements-dev.txt        2024-10-27 18:48:15
+black==24.10.0
+mypy==1.13.0
+mypy-extensions==1.0.0
+packaging==24.1
+pathspec==0.12.1
+platformdirs==4.3.6
+ruff==0.7.1

I will include the docs for the new functionality and tests, I'm trying to keep style as similar as I can, feel free to nitpick.
For the comments, we can go case by case, I will adding comments when pushing code/fixes, so you can check and resolve if good! =]

@kiraum kiraum force-pushed the main branch 6 times, most recently from 6fe8ef9 to 8579672 Compare October 27, 2024 19:51
@alexpovel
Copy link
Owner

Thanks for the quick answer/comments, I will go over it during the week, a bit slow here, because it's herfstvakantie (autumn holidays), so a bit busy with family! 😅

Sounds great! Let's take all the time we like, this is about having fun!

[project.optional-dependencies]

I think I prefer uv's approach. They give a good reason:

Unlike optional dependencies, development dependencies are local-only and will not be included in the project requirements when published to PyPI or other indexes. As such, development dependencies are not included in the [project] table.

which is what we want. Those dependencies shouldn't occur anywhere but for local dev, and in CI. With optional deps, users could pip install ancv[dev] if I read that guide right, which doesn't seem right.

@alexpovel
Copy link
Owner

Just discovered: I refactored to use uv just days ago, and used

ancv/pyproject.toml

Lines 53 to 67 in e60f5f5

[tool.uv]
dev-dependencies = [
"datamodel-code-generator[http]>=0.26.2",
"ipython>=8.28.0",
"mypy>=1.13.0",
"pydeps>=2.0.1",
"pytest-aiohttp>=1.0.5",
"pytest-cov>=5.0.0",
"pytest-asyncio>=0.24.0",
"pytest-rerunfailures>=14.0",
"pytest>=8.3.3",
"requests>=2.32.3",
"types-babel>=2.11.0.15",
"types-cachetools>=5.5.0.20240820",
]

which is already outdated practice...:

https://github.com/astral-sh/uv/releases/tag/0.4.27

aka PEP 735 will allow this to be expressed natively and standards-based, which is fantastic:

[dependency-groups]
dev = [
  "pytest >=8.1.1,<9"
]

etc.

But 0.4.27 isn't packaged for devbox/nix yet, so can't and don't want to rely on that just yet.

@kiraum
Copy link
Contributor Author

kiraum commented Oct 27, 2024

Just discovered: I refactored to use uv just days ago, and used

ancv/pyproject.toml

Lines 53 to 67 in e60f5f5

[tool.uv]
dev-dependencies = [
"datamodel-code-generator[http]>=0.26.2",
"ipython>=8.28.0",
"mypy>=1.13.0",
"pydeps>=2.0.1",
"pytest-aiohttp>=1.0.5",
"pytest-cov>=5.0.0",
"pytest-asyncio>=0.24.0",
"pytest-rerunfailures>=14.0",
"pytest>=8.3.3",
"requests>=2.32.3",
"types-babel>=2.11.0.15",
"types-cachetools>=5.5.0.20240820",
]

which is already outdated practice...:

https://github.com/astral-sh/uv/releases/tag/0.4.27

aka PEP 735 will allow this to be expressed natively and standards-based, which is fantastic:

[dependency-groups]
dev = [
  "pytest >=8.1.1,<9"
]

etc.

But 0.4.27 isn't packaged for devbox/nix yet, so can't and don't want to rely on that just yet.

yeah, uv release is too fast, hard to follow 😅

I will give a try to that myself this week!

@kiraum kiraum force-pushed the main branch 2 times, most recently from a93e2cc to 2c91583 Compare November 14, 2024 19:28
@kiraum
Copy link
Contributor Author

kiraum commented Nov 14, 2024

@alexpovel , sorry for the delay, busy days here... company announced reorg, so a lot happening! 😅

Added a section about the new function in the README.md, and tests to verify that WebHandler correctly fetches, caches and serves resume data from a URL while handling both successful and error scenarios.

I checked ruff check/format, mypy, and pytest, results looks good, can you please take a look when possible? If good, can run the checks again?

Copy link
Owner

@alexpovel alexpovel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, no worries!

It's looking good, thanks for adding tests and docs. I just have comments on some of the error handling (and tests thereof). Then it's good to go!

tests/web/test_server.py Show resolved Hide resolved
tests/web/test_server.py Show resolved Hide resolved
tests/web/test_server.py Outdated Show resolved Hide resolved
ancv/web/server.py Outdated Show resolved Hide resolved
ancv/web/server.py Outdated Show resolved Hide resolved
ancv/web/server.py Fixed Show resolved Hide resolved
@alexpovel
Copy link
Owner

https://github.com/alexpovel/ancv/actions/runs/11845227596/job/33079082336?pr=241#step:7:1315

These errors are on me, sorry ☹️ Thought I fixed those. It's something about PRs and having access to secrets... I'm happy to test your PR locally and then merge anyway

@kiraum
Copy link
Contributor Author

kiraum commented Nov 16, 2024

No problems, and don't worry, really appreciate the comments/time to review it, and I always do basic tests locally before to push, so they should be good..... in theory 😅

ancv/web/server.py Fixed Show resolved Hide resolved
@alexpovel
Copy link
Owner

Thanks! Look good. There's still the str(exc) bit left GitHub is warning about. You mentioned you addressed it, perhaps just forgot to push?

@kiraum
Copy link
Contributor Author

kiraum commented Nov 17, 2024

My bad, just pushed it, should be good now! =]

@alexpovel alexpovel merged commit 51e1640 into alexpovel:main Nov 18, 2024
5 of 6 checks passed
@alexpovel
Copy link
Owner

Thank you for your contribution @kiraum ! I just merged... let's see what fails in CI now. I need to fix that flakiness...

@kiraum
Copy link
Contributor Author

kiraum commented Nov 18, 2024

Thanks! My pleasure, learning a lot here! =]

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