-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix: EOL filtering to only exclude builds with no valid tracks. #272
Conversation
f5af210
to
ae87140
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. Nicely done. A few nitpicks regarding the formatting. I would also suggest use isort
to sort the imports ;)
…factory into fix_eol_filtering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will omit the blank line after the docstring in this PR as per the discussion offline. Looks good to me. Thanks.
@@ -67,21 +67,26 @@ | |||
|
|||
print(f"Parsing image trigger {args.image_trigger}") | |||
with open(args.image_trigger, encoding="UTF-8") as trigger: | |||
image_trigger = ImageSchema(**yaml.load(trigger, Loader=yaml.BaseLoader)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey,
Just to know, Why changing this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to make some changes when updating to pydantic v2. L78 was failing on both the old .dict
and new .dump_model
methods, so I just kept the image_trigger
as a dict, and use the model only for validation like we do in most of the repo.
78: for risk, value in risks.dict(exclude_none=True).items():
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me. Once these nitpicks are fixed, I can approved this PR.
pydantic==2.8.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pydantic==2.8.2 | |
pydantic==2.8.2 | |
from typing import Any | ||
from pathlib import Path | ||
from git import Repo | ||
from tempfile import TemporaryDirectory as tempdir | ||
from copy import deepcopy | ||
import logging | ||
|
||
|
||
from ..uploads.infer_image_track import get_base_and_track | ||
from ..shared.github_output import GithubOutput | ||
from .utils.schema.triggers import ImageSchema | ||
from .utils.schema.revision_data import RevisionDataSchema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use isort
to sort the imports?
Included in this PR:
2.8.2
globally.