Conversation
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
theFong
requested changes
Aug 5, 2025
Member
theFong
left a comment
There was a problem hiding this comment.
remove all un-useful methods and files.
internal/fluidstack/v1/client.go
Outdated
| } | ||
| } | ||
|
|
||
| func (c *FluidStackClient) GetCapabilities(ctx context.Context) (v1.Capabilities, error) { |
Member
There was a problem hiding this comment.
lets move this to the capabilities.go and remove the Project capabilities
theFong
requested changes
Aug 5, 2025
Member
theFong
left a comment
There was a problem hiding this comment.
please remove the unnecessary no implemented functions. Does fluidstack support images?
internal/fluidstack/v1/client.go
Outdated
|
|
||
| var _ v1.CloudClient = &FluidStackClient{} | ||
|
|
||
| func NewFluidStackClient(apiKey string) *FluidStackClient { |
Member
There was a problem hiding this comment.
lets make a cloud cred that turns into a client. If you rebase lambda labs you will see how we implemented there.
- Implement FluidStackClient with all CloudClient interface methods - Add project management APIs (create, list, get, delete projects) - Include comprehensive security documentation with disk encryption analysis - Document API capabilities and limitations in README - Follow Lambda Labs provider pattern for consistency - All methods return v1.ErrNotImplemented as stubs for future implementation Features: - Hardware-level disk encryption (self-encrypting drives) - Project-level network isolation using VXLAN/eBPF - Instance lifecycle management (create, start, stop, terminate) - Instance type discovery and tagging support - Comprehensive compliance certifications (HIPAA, GDPR, ISO27001, SOC 2) Limitations: - No individual instance firewall rule management - Security managed at project/cluster level - Limited granular network control APIs Co-Authored-By: Alec Fong <alecsanf@usc.edu>
- Rename all unused parameters with underscore prefix to satisfy revive linter - Fix gofumpt formatting issues in client.go struct initialization - Ensure all CloudClient interface methods comply with linting standards - All files now pass golangci-lint checks successfully Co-Authored-By: Alec Fong <alecsanf@usc.edu>
- Remove unnecessary files: project.go, quota.go, storage.go, image.go, networking.go - Move GetCapabilities method from client.go to capabilities.go - Remove Project-related capabilities (create/delete/list/get project) - Simplify FluidStackClient struct to match Lambda Labs pattern - Remove unnecessary methods: GetName(), GetRegions(), StartInstance(), StopInstance(), ChangeInstanceType(), UpdateInstanceTags() - Keep only essential CloudClient interface methods for basic provider functionality Addresses GitHub PR feedback from @theFong Co-Authored-By: Alec Fong <alecsanf@usc.edu>
- Add FluidStackCredential struct implementing CloudCredential interface - Update FluidStackClient to be created from credential with refID parameter - Add GetCapabilities method to credential that delegates to client - Implement GetTenantID with SHA256 hash of API key following Lambda Labs pattern - Update NewFluidStackClient to accept both refID and apiKey parameters - Remove GetTenantID from client since it's now handled by credential Addresses GitHub PR feedback from @theFong about implementing cloud credential pattern. FluidStack does support images as confirmed from API documentation (image parameter with default 'image://ubuntu22.04'). Co-Authored-By: Alec Fong <alecsanf@usc.edu>
fbbd3c1 to
3f554ae
Compare
theFong
requested changes
Aug 5, 2025
internal/fluidstack/v1/client.go
Outdated
|
|
||
| // GetCloudProviderID returns the cloud provider ID for FluidStack | ||
| func (c *FluidStackCredential) GetCloudProviderID() v1.CloudProviderID { | ||
| return "fluidstack" |
- Add const CloudProviderID = "fluidstack" at package level - Update both GetCloudProviderID() methods to use the constant - Update GetTenantID() to use constant in format string for consistency Addresses GitHub PR comment from @theFong requesting to make the hardcoded "fluidstack" string a constant. Co-Authored-By: Alec Fong <alecsanf@usc.edu>
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.
Implement cloud credential pattern for FluidStack provider
Summary
This PR implements the cloud credential pattern for the FluidStack provider to match the established Lambda Labs approach. The key change separates credential management from client operations by introducing a
FluidStackCredentialstruct that implements theCloudCredentialinterface and createsFluidStackClientinstances viaMakeClient().Key Changes:
FluidStackCredentialstruct implementingCloudCredentialinterface with RefID/APIKey fieldsFluidStackClientconstructor to accept bothrefIDandapiKeyparameters (was previously justapiKey)GetCapabilities()methodimageparameter with default"image://ubuntu22.04")This addresses GitHub PR feedback from @theFong requesting the cloud credential pattern and clarification on image support.
Review & Testing Checklist for Human
FluidStackCredentialandFluidStackClientproperly implement their respective interfaces by runninggo build ./internal/fluidstack/v1/...and checking for compilation errorsNewFluidStackCredential().MakeClient()creates a working client and thatGetReferenceID()returns the correct RefIDinternal/fluidstack/v1/client.govsinternal/lambdalabs/v1/client.goto ensure structural consistencyFluidStackCredential.GetCapabilities()properly delegates to the client without circular dependencies or infinite loopsRecommended Test Plan: Create a
FluidStackCredentialinstance, callMakeClient(), verify the resulting client has correct RefID, and test thatGetCapabilities()works without errors.Diagram
%%{ init : { "theme" : "default" }}%% graph TD subgraph "FluidStack Provider" FSClient["internal/fluidstack/v1/<br/>client.go"]:::major-edit FSCaps["internal/fluidstack/v1/<br/>capabilities.go"]:::context FSInstance["internal/fluidstack/v1/<br/>instance.go"]:::context FSInstanceType["internal/fluidstack/v1/<br/>instancetype.go"]:::context end subgraph "Core Interfaces" CloudCred["pkg/v1/client.go<br/>CloudCredential"]:::context CloudClient["pkg/v1/client.go<br/>CloudClient"]:::context end subgraph "Reference Pattern" LambdaClient["internal/lambdalabs/v1/<br/>client.go"]:::context end subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end FSClient -->|implements| CloudCred FSClient -->|implements| CloudClient FSClient -->|follows pattern| LambdaClient FSClient -->|delegates to| FSCaps classDef major-edit fill:#90EE90 classDef minor-edit fill:#87CEEB classDef context fill:#FFFFFFNotes
FluidStackCredentialandFluidStackClientmust implement complex interfaces - compilation success is necessary but manual verification of behavior is recommendedFluidStackCredential.GetCapabilities()creates a temporary client to fetch capabilities - this should be tested to ensure no circular dependenciesSession Details: Requested by Alec Fong (@theFong)
Link to Devin run: https://app.devin.ai/sessions/338c2a2ea6d34fd3ba76477ea88c47e6