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.

Sign up for GitHub

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

Jump to bottom

add pagination for artifacts retrieval #453

Merged
merged 2 commits into from
Aug 8, 2024
Merged

add pagination for artifacts retrieval #453

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0951563
add pagination for artifacts retrieval
fabiogallotti Aug 4, 2024
ac3108e
improvements on pagination after code review
fabiogallotti Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,6 @@
.coverage
dev-env-vars
.coverage*
.python-version
__pycache__
.vscode
Copy link
Member

Choose a reason for hiding this comment

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

I agree __pycache__ is missing

For the 2 others, it seem they would belong to your personal global gitignore. It's not that we couldn't do it here, but whenever people suggest we add those (which happens once a year) I'd rather show the proper way of dealing with those and avoid similar suggestions on other repos. That said, let me emphasize you did nothing wrong by suggesting those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks for letting me know this

21 changes: 21 additions & 0 deletions Makefile
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of Makefiles, mainly because they tend to look like a mess (not commenting your Makefile in particular, but it's an inherent property of the format). Also, they lost me many years ago when I discovered you have to mix tabs and space on some lines.

My philosophy is more to try and use standard tools and configure them so they're ready to use.

In the case of your Makefile: if you think the project is missing some directions on how to launch some actions, you're right of course, but I'd like to direct you to changing the CONTRIBUTING.md doc first. That said, I'm totally ok to keep the Makefile as a documentation TL;DR, but let's make it so (remove the help target, fix the few comments I left which should show that targets are synonym to a 1 (or few) words command.

If you're curious, my go-to approach these days when I need to make complex commands accessible in a project is a scripts folder, which offers the advantage of letting you write some scripts with Python when they become too complex to be readable in bash. But I don't think we need this here. A Makefile will be very OK.

Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
.DEFAULT_GOAL := help
SHELL = bash


.PHONY: help
help: ## show this help
@grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}'

.PHONY: install
install: ## install dependencies
poetry install --with dev
poetry run pre-commit install
Copy link
Member

Choose a reason for hiding this comment

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

pre-commit shouldn't be installed inside poetry, so that's rather pre-commit install without poetry run


.PHONY: lint
lint: ## lint code
poetry run ruff check --fix coverage_comment tests
poetry run ruff format coverage_comment tests
Copy link
Member

Choose a reason for hiding this comment

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

Pre-commit takes care of linting. A more appropriate (and more complete) lint command would be

pre-commit run --all-files

Or even just

pre-commit


.PHONY: test
test: ## run all tests
poetry run pytest tests -vv --cov-report term-missing --cov=coverage_comment
Copy link
Member

Choose a reason for hiding this comment

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

I believe all those arguments are already in pyproject.toml, no need to repeat them here.

9 changes: 8 additions & 1 deletion coverage_comment/github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,14 @@ def download_artifact(
filename: pathlib.Path,
) -> str:
repo_path = github.repos(repository)
artifacts = repo_path.actions.runs(run_id).artifacts.get().artifacts
page = 1
artifacts = []
while True:
result = repo_path.actions.runs(run_id).artifacts.get(page=str(page))
if not result:
break
artifacts.extend(result.artifacts)
page += 1
try:
artifact = next(
Copy link
Member

@ewjoachim ewjoachim Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What about incorporating reading the pages and paginating into one generator (yield), so that if you find the artifact on the first page, you don't need to fetch the second page ? Also, this will limit the number of tests you have to change while still letting you do dedicated tests for pagination.

Also, if the number of elements github returns on a page is fewer than the max, you know in advance there won't be a next page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, I'll work on improving it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid unnecessarily fetching the next page, I leveraged on the total_count attribute of the response, instead of using the github default max page size of 30, it seems safer to me

iter(artifact for artifact in artifacts if artifact.name == artifact_name),
Expand Down
52 changes: 52 additions & 0 deletions tests/integration/test_github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,48 @@ def test_download_artifact(gh, session, zip_bytes):
session.register("GET", "/repos/foo/bar/actions/runs/123/artifacts")(
json={"artifacts": artifacts}
)
session.register(
"GET",
"/repos/foo/bar/actions/runs/123/artifacts",
params={"page": "2"},
)(json={})

session.register("GET", "/repos/foo/bar/actions/artifacts/789/zip")(
content=zip_bytes(filename="foo.txt", content="bar")
)

result = github.download_artifact(
github=gh,
repository="foo/bar",
artifact_name="foo",
run_id=123,
filename=pathlib.Path("foo.txt"),
)

assert result == "bar"


def test_download_artifact_from_page_2(gh, session, zip_bytes):
artifacts_page_1 = [
{"name": "test", "id": 000},
]
artifacts_page_2 = [
{"name": "bar", "id": 456},
{"name": "foo", "id": 789},
]
session.register("GET", "/repos/foo/bar/actions/runs/123/artifacts")(
json={"artifacts": artifacts_page_1}
)
session.register(
"GET",
"/repos/foo/bar/actions/runs/123/artifacts",
params={"page": "2"},
)(json={"artifacts": artifacts_page_2})
session.register(
"GET",
"/repos/foo/bar/actions/runs/123/artifacts",
params={"page": "3"},
)(json={})

session.register("GET", "/repos/foo/bar/actions/artifacts/789/zip")(
content=zip_bytes(filename="foo.txt", content="bar")
Expand All @@ -77,6 +119,11 @@ def test_download_artifact__no_artifact(gh, session):
session.register("GET", "/repos/foo/bar/actions/runs/123/artifacts")(
json={"artifacts": artifacts}
)
session.register(
"GET",
"/repos/foo/bar/actions/runs/123/artifacts",
params={"page": "2"},
)(json={})

with pytest.raises(github.NoArtifact):
github.download_artifact(
Expand All @@ -95,6 +142,11 @@ def test_download_artifact__no_file(gh, session, zip_bytes):
session.register("GET", "/repos/foo/bar/actions/runs/123/artifacts")(
json={"artifacts": artifacts}
)
session.register(
"GET",
"/repos/foo/bar/actions/runs/123/artifacts",
params={"page": "2"},
)(json={})

session.register("GET", "/repos/foo/bar/actions/artifacts/789/zip")(
content=zip_bytes(filename="foo.txt", content="bar")
Expand Down
10 changes: 10 additions & 0 deletions tests/integration/test_main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,6 +812,11 @@ def test_action__workflow_run__no_artifact(
"GET",
"/repos/py-cov-action/foobar/actions/runs/123/artifacts",
)(json={"artifacts": [{"name": "wrong_name"}]})
session.register(
"GET",
"/repos/py-cov-action/foobar/actions/runs/123/artifacts",
params={"page": "2"},
)(json={})

result = main.action(
config=workflow_run_config(),
Expand Down Expand Up @@ -853,6 +858,11 @@ def test_action__workflow_run__post_comment(
"GET",
"/repos/py-cov-action/foobar/actions/runs/123/artifacts",
)(json={"artifacts": [{"name": "python-coverage-comment-action", "id": 789}]})
session.register(
"GET",
"/repos/py-cov-action/foobar/actions/runs/123/artifacts",
params={"page": "2"},
)(json={})

session.register(
"GET",
Expand Down
Loading