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 issue #758. #832

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Fix issue #758. #832

merged 1 commit into from
Mar 21, 2024

Conversation

bdice
Copy link
Member

@bdice bdice commented Mar 18, 2024

Description

This PR attempts to fix #758.

Motivation and Context

Currently bundles are considered for status check operations as long as they start with a given prefix. This prefix is not uniquely identifying (through the inclusion of a hash, for example), because it is meant to be short and readable. However, this can cause projects to try to read status updates from other FlowProject instances (and potentially other users) if they share a common FlowProject class name.

This fixes #758 by only considering bundles that already exist within the project.

Note: These bundle ids are guaranteed to be unique (up to hash collisions) because they include the project's filesystem path in a hash when computing the id of each _JobOperation. These _JobOperation ids are then hashed together to produce a unique bundle id.

I do not have a way to test this PR on a scheduler. Support from reviewers in testing would be welcome.

Checklist:

Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

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

Thanks! This turned out to be a simple fix.

As you say, we should test on a cluster before merging.

@CalCraven
Copy link

Tested this using the example I posted in #758. Works like a charm, thanks @bdice!

@bdice
Copy link
Member Author

bdice commented Mar 21, 2024

Great. @cbkerr I'll let you merge since you're taking care of the release in #833. Please add a changelog entry in the release PR.

@cbkerr
Copy link
Member

cbkerr commented Mar 21, 2024

@CalCraven Thanks for testing! I'll put it in the release

@cbkerr cbkerr merged commit ac6a5ac into main Mar 21, 2024
9 checks passed
@cbkerr cbkerr deleted the fix/758 branch March 21, 2024 19:20
@cbkerr cbkerr added this to the v0.29.0 milestone Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants