Skip to content

Reuse TowerClient to handle Fusion token requests#6207

Merged
pditommaso merged 9 commits intomasterfrom
fix-refresh-license-token
Jun 30, 2025
Merged

Reuse TowerClient to handle Fusion token requests#6207
pditommaso merged 9 commits intomasterfrom
fix-refresh-license-token

Conversation

@jordeu
Copy link
Collaborator

@jordeu jordeu commented Jun 23, 2025 •
edited by pditommaso
Loading

Overview

This PR reuses the TowerClient logic to handle all license token requests, which will avoid the code from diverging in the future and fix the missing refresh token issue in the current implementation.

Implementation details

This pull request introduces significant changes to the TowerFusionToken class and its related components, focusing on simplifying the codebase and improving integration with the TowerClient. The updates remove redundant HTTP handling logic, streamline configuration validation, and enhance test coverage. Additionally, a new method is added to serialize license token requests into a map format.

Codebase Simplification:

  • Integration with TowerClient: The TowerFusionToken class now uses the TowerClient for HTTP requests, removing custom HTTP client logic, retry policies, and related helper methods. This simplifies the code and centralizes HTTP handling. (plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionToken.groovy, [1] [2]
  • Validation Removal: Configuration validation logic for endpoint and accessToken has been removed, relying instead on TowerClient to handle these concerns. (plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionToken.groovy, [1] [2]

Feature Enhancements:

Test Updates:

  • Test Refactoring: Removed tests that validated endpoint and accessToken extraction logic, as these functionalities are now handled by TowerClient. New tests were added to validate workflow creation and token retrieval using the updated TowerFusionToken implementation. (plugins/nf-tower/src/test/io/seqera/tower/plugin/TowerFusionEnvTest.groovy, plugins/nf-tower/src/test/io/seqera/tower/plugin/TowerFusionEnvTest.groovyL46-R91)

@netlify
Copy link

netlify bot commented Jun 23, 2025 •
edited
Loading

✅ Deploy Preview for nextflow-docs-staging canceled.

Name Link
🔨 Latest commit ac3de33
🔍 Latest deploy log https://app.netlify.com/projects/nextflow-docs-staging/deploys/686240a44d787b0008ccf8be

pditommaso
pditommaso reviewed Jun 23, 2025
View reviewed changes
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.

Not sure this can easily back-ported to older branches

@jordeu
Copy link
Collaborator Author

jordeu commented Jun 23, 2025

Not sure this can easily back-ported to older branches

I checked with 24.04, and it is no problem to backport it because the TowerClient and TowerFactory interfaces are the same.

jordeu added 4 commits June 27, 2025 16:59
@jordeu
remove unused method
a2f62c1 
Signed-off-by: Jordi Deu-Pons <jordi@jordeu.net>
@jordeu
Use TowerClient to handle all token license requests
c248495 
Signed-off-by: Jordi Deu-Pons <jordi@jordeu.net>
@jordeu
Fix TowerFusionEnvTest tests
8cc5bb6 
Signed-off-by: Jordi Deu-Pons <jordi@jordeu.net>
@jordeu
Remove unused imports
475f64e 
Signed-off-by: Jordi Deu-Pons <jordi@jordeu.net>
@jordeu jordeu force-pushed the fix-refresh-license-token branch from 365f35a to 475f64e Compare June 27, 2025 14:59
@pditommaso
Copy link
Member

pditommaso commented Jun 28, 2025

Thanks for looking into this @jordeu, it looks good however I think it would be better to keep using TowerClient exclusively for Platform telemetry, and do not use for other tasks.

The main raison is that, in principle, Fusion can be used even without activating Platform telemetry (provided the access token is provided). This PR instead, implicitly enables the telemetry even if not required.

https://github.com/nextflow-io/nextflow/pull/6207/files#diff-a253f546ca68a21c3b2176ffa1135ab6b87d9b63215c94fbb2cca1cfe95be128R103

I've opened a separate PR to add the JWT token logic in the current implementation #6231

It's true that there's some code duplication, however this already a problem for other clients. see this and this, other than this.

For this problem we may introduce a new http client class dedicated to properly handle this need in across all implementations.

@pditommaso pditommaso closed this Jun 28, 2025
@pditommaso pditommaso reopened this Jun 30, 2025
@pditommaso pditommaso changed the title Reuse TowerClient to handle all license token requests Reuse TowerClient to handle Fusion token requests Jun 30, 2025
@pditommaso
Just add return [ci skip]
66ea481 
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso
Use explicit session and env in tests [ci fast]
ac3de33 
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso
Copy link
Member

pditommaso commented Jun 30, 2025

It turns our Platform telemetry is a desired requirement. Therefore this implementation makes more sense.

@pditommaso pditommaso merged commit 9883b4d into master Jun 30, 2025
9 checks passed
@pditommaso pditommaso deleted the fix-refresh-license-token branch June 30, 2025 09:10
@pditommaso pditommaso mentioned this pull request Jun 30, 2025
Merged
@pditommaso
Copy link
Member

pditommaso commented Jun 30, 2025

Ouch getting this in the CI tests

WARN: Unable to serialize key=; value=[product:fusion, version:2.4, workflowId:null, workspaceId:null]; type=java.util.HashMap -- Cause: Index: 1, Size: 1
[fe/2ee5bc] Submitted process > RNASEQ:FASTQC (FASTQC on lung)
WARN: Unable to validate Fusion license - reason: Invalid response: https://api.cloud.seqera.io/license/token/ [400] Unexpected error while processing request - Error ID: 2d2bVF6Gnd9EEIsjvwpCBr -- {"message":"Unexpected error while processing request - Error ID: 2d2bVF6Gnd9EEIsjvwpCBr"}

pditommaso added a commit that referenced this pull request Jun 30, 2025
@pditommaso
Revert "Fix JWT refresh and use TowerClient to handle Fusion token re…
b8a7722 
…quests #6207" [ci fast]

This reverts commit 9883b4d.

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso
Copy link
Member

pditommaso commented Jun 30, 2025

I've reverted this PR 👉 b8a7722

exthnet pushed a commit to exthnet/nextflow_tcs that referenced this pull request Jul 3, 2025
@pditommaso @exthnet
Revert "Fix JWT refresh and use TowerClient to handle Fusion token re…
470a055 
…quests nextflow-io#6207" [ci fast]

This reverts commit 9883b4d.

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: Satoshi OHSHIMA <ohshima@cc.kyushu-u.ac.jp>
@jorgee jorgee mentioned this pull request Oct 28, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pditommaso pditommaso pditommaso approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jordeu @pditommaso