Refactor: decouple COS and auth from FleetHandler#2108
Merged
Conversation
… for both. Fixes a subtle bug where Configuration's TypeWithDefault metaclass returns a shallow copy.
…re-built ce_api_client (ApiClient) instead of building one internally from IBMCloudClientProvider. JobCOS is decoupled from FleetHandler: it now wraps a COSClient directly and no longer holds a reference to the handler. build_cos_client() is a standalone factory that resolves HMAC credentials from the CE secret and returns a ready JobCOS.
Tansito
requested changes
May 14, 2026
Member
Tansito
left a comment
There was a problem hiding this comment.
Thanks for the PR @ElePT in essence I think it's the correct path to follow! A couple of minor comments that doesn't change the implementation but I think they are worth it:
- We are building factories in the
__init__files to avoid local imports like the one we have inbuild_ce_auth. You can use get_runner or get_arguments_storage as an example. - I would like to propose change
build_ce_authandbuild_cos_clientnames byget_*to maintain consistency along all our factories. - Maybe not for this PR, totally up to you here, I saw that in
build_ce_authwe are returning aTuple. Maybe it's better to return an object in this case as it can be confusing that a Factory returns a tuple here (it's also something we can do later if we want to move forward with this, no problem, it almost a refactor).
Collaborator
Author
|
Suggestions applied @Tansito, including the refactoring you suggested (returning a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FleetHandlerwas doing too much: auth setup, COS credential resolution, and CE API calls. This PR splits those responsibilities apart.build_ce_authis a new shared factory inclients.pythat returns a ready(ApiClient, IBMCloudClientProvider). On the COS side,build_cos_client(project)is a standalone factory that fetches HMAC credentials from the CE secret and returns a readyJobCOS.FleetsRunnerinitializes the two independently via_get_handler()and_get_cos().As part of this PR work I found that the swagger
Configurationclass used a shallow-copy singleton, so multipleFleetHandlerinstances were mutating each other's IAM token dict. This is now fixed by assigning fresh dicts instead of mutating.FleetHandlernow just takes the pre-builtApiClientand stays a pure CE API wrapperDetails and comments
I am leaving the PR as a draft because I am having issues testing it on the demo branch, I will make it ready for review once I have verified that it works.
The goal of this and the follow-up PR is to enable the following flow at job creation time (rough implementation) without unnecessarily calling the
FleetsRunneror theFleetHandler: