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

fix: get_ancestors should ignore 404s #569

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

bhearsum
Copy link
Contributor

@bhearsum bhearsum commented Sep 3, 2024

This can happen when attempting to fetch tasks (either directly or indirectly) that have expired. My view on this is that such tasks should be treated as no longer being dependencies since they no longer exist.

I can see a case for maybe making this optional behaviour, but I don't know of any use cases where we'd want to raise an exception for this case. Certainly when being used as part of actions or other CI code it ends up being a ticking time bomb.

This can happen when attempting to fetch tasks (either directly or indirectly) that have expired. My view on this is that such tasks should be treated as no longer being dependencies since they no longer exist.

I can see a case for maybe making this optional behaviour, but I don't know of any use cases where we'd want to raise an exception for this case. Certainly when being used as part of actions or other CI code it ends up being a ticking time bomb.
@bhearsum bhearsum marked this pull request as ready for review September 3, 2024 19:52
@bhearsum bhearsum requested review from a team and ahal September 3, 2024 19:57
Copy link
Contributor

@hneiva hneiva left a comment

Choose a reason for hiding this comment

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

I agree that this should be the expected behaviour.

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.

Makes sense to me, I'm surprised we haven't hit this before now.

We could return a tuple with expired_ancestors being the second variable.. But it's probably not worth the breaking change. Also it wouldn't even be fully accurate as we'd have no way of finding dependencies of the expired tasks in turn.

@bhearsum bhearsum merged commit 5b9cd0a into taskcluster:main Sep 4, 2024
15 checks passed
bhearsum added a commit to bhearsum/firefox-translations-training that referenced this pull request Sep 5, 2024
This includes taskcluster/taskgraph#569, which is the upstreamed version of the fix for taskcluster/taskgraph#569.
bhearsum added a commit to bhearsum/firefox-translations-training that referenced this pull request Sep 5, 2024
This includes taskcluster/taskgraph#569, which is the upstreamed version of the fix for taskcluster/taskgraph#569.
bhearsum added a commit to bhearsum/firefox-translations-training that referenced this pull request Sep 5, 2024
This includes taskcluster/taskgraph#569, which is the upstreamed version of the fix for taskcluster/taskgraph#569.
bhearsum added a commit to mozilla/translations that referenced this pull request Sep 6, 2024
This includes taskcluster/taskgraph#569, which is the upstreamed version of the fix for taskcluster/taskgraph#569.
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.

3 participants