Skip to content

Conversation

@bentsherman
Copy link
Member

This PR moves the task hashing logic from TaskProcessor to a separate class TaskHasher

  • Makes the TaskProcessor easier to read
  • Sets us up for a v2 task hash in the future if we need it (e.g. for lineage)

@bentsherman bentsherman requested a review from jorgee November 14, 2025 18:53
@netlify
Copy link

netlify bot commented Nov 14, 2025

Deploy Preview for nextflow-docs-staging canceled.

Name Link
🔨 Latest commit ee02d3a
🔍 Latest deploy log https://app.netlify.com/projects/nextflow-docs-staging/deploys/69177acbf4d88b000886b50e

}


def 'should get bin files in the script command' () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this test has been removed. It doesn't seem to be related to TaskHasher.

Copy link
Member Author

Choose a reason for hiding this comment

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

Both of these tests were moved to the TaskHasherTest and refactored. This is because the bin scripts and task directive vars are only used to calculate the task hash.

I believe the only other place these methods are used is in the LinObserver, and that's only to preserve the hash components in the TaskRun lineage record


}

def 'should get task directive vars' () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before, it doesn't seem to be related with task hasher.

Copy link
Member

@pditommaso pditommaso left a comment

Choose a reason for hiding this comment

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

Bit against reshuffling code without a clear motivation.

@bentsherman
Copy link
Member Author

The motivation is in the PR description

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.

4 participants