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

Allow expiration to be overridden in fetch and docker-image kinds. Allow setting the default expiry on "try" tasks #409

Merged
merged 7 commits into from
Jan 5, 2024

Conversation

gabrielBusta
Copy link
Member

@gabrielBusta gabrielBusta commented Jan 4, 2024

fixes #392
fixes #393
fixes #350
fixes #351

Copy link

codecov bot commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (29ebb22) 60.06% compared to head (4131a6c) 60.06%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #409   +/-   ##
=======================================
  Coverage   60.06%   60.06%           
=======================================
  Files          69       69           
  Lines        6360     6361    +1     
  Branches     1284     1284           
=======================================
+ Hits         3820     3821    +1     
  Misses       2254     2254           
  Partials      286      286           

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

@gabrielBusta gabrielBusta requested a review from ahal January 4, 2024 17:24
Copy link
Collaborator

@ahal ahal left a comment

Choose a reason for hiding this comment

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

I think I'd prefer a name like default-task-expiry, but I see we already have task-deadline-after, so this is fine as it's better to be consistent.

src/taskgraph/transforms/docker_image.py Outdated Show resolved Hide resolved
src/taskgraph/transforms/docker_image.py Outdated Show resolved Hide resolved
src/taskgraph/transforms/fetch.py Outdated Show resolved Hide resolved
@ahal
Copy link
Collaborator

ahal commented Jan 4, 2024

Also bonus points for adding some tests to test_transforms_fetch.py and test_transforms_docker_image.py to fix the coverage :)

@gabrielBusta gabrielBusta changed the title Allow fetch expiration to be overridden in kinds Allow expiration to be overridden in fetch and docker-image kinds. Allow setting the default expiry on "try" tasks Jan 4, 2024
@gabrielBusta gabrielBusta requested a review from ahal January 4, 2024 21:02
@gabrielBusta
Copy link
Member Author

Thanks! I improved the patch.

I am gonna delete so much code from the translations repo when this lands :)

@gabrielBusta
Copy link
Member Author

I think I'd prefer a name like default-task-expiry, but I see we already have task-deadline-after, so this is fine as it's better to be consistent.

Yeah, I was going for consistency there.

"task-expires-after",
description="Default 'expires-after' for non level 3 tasks, in relative date format. "
"Eg: '90 days'",
): str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think saying "non level 3" is technically incorrect because is_try() only catches pull requests and projects that contain the string "try". Saying "level 1 tasks" would be more accurate.

That said, it's kind of a shame that this only impacts level 1 tasks. What if we added an optionally_keyed_by("level", str) (like how task-deadline-after is optionally keyed by project above).

This way the graph config could look like:

task-expires-after:
    by-level:
        1: 90 days
        3: 1 year

Also, you could just set a single default for all levels:

task-expires-after: 1 year

Feel free to leave as a follow-up, but would be nice to have an issue on file at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked into it a bit and filed #410

@gabrielBusta gabrielBusta merged commit 551c90d into main Jan 5, 2024
10 checks passed
@gabrielBusta gabrielBusta deleted the issue-392 branch January 5, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants