Skip to content

Conversation

devin-ai-integration[bot]
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Aug 9, 2025

Add comprehensive testing and implement GitHub feedback for CLI credential system

Summary

This PR adds extensive testing to the bcloud CLI configuration system and implements feedback to standardize credential field naming and provider IDs. The changes ensure robust error handling and edge case coverage for credential loading while maintaining backward compatibility patterns.

Key Changes:

  • Added comprehensive test suite for LoadConfig() function covering success cases, fallback scenarios, and error conditions
  • Added JSON tags to provider credential structs (LambdaLabsCredential, NebiusCredential) with standardized lowercase field names
  • Unified Lambda Labs provider ID from "lambda-labs" to "lambdalabs" to match registry key
  • Removed provider ID mapping logic and added provider constants to eliminate lint warnings
  • Added tests for credential parsing, marshaling/unmarshaling, and provider registry functionality

Review & Testing Checklist for Human

Critical Items (3):

  • Test end-to-end with real Lambda Labs credentials - Create a config file with actual API keys and verify bcloud commands work against live Lambda Labs API
  • Verify provider ID change doesn't break existing systems - Check if any existing configs or external systems depend on the old "lambda-labs" provider ID
  • Test credential file loading from both locations - Verify ~/.bcloud/credentials.yaml and ./bcloud.yaml fallback works correctly with real config files

Important Items (2):

  • Validate key-as-ref_id pattern - Test that YAML keys properly become ref_id when not explicitly set, and explicit ref_id overrides work
  • Check JSON tag impact on SDK - Ensure the added JSON tags to credential structs don't affect other parts of the SDK that serialize these types

Diagram

%%{ init : { "theme" : "default" }}%%
graph TD
    subgraph "CLI System"
        A["cmd/bcloud/config/config.go"]:::major-edit
        B["cmd/bcloud/config/config_test.go"]:::major-edit
        C["cmd/bcloud/commands/create.go"]:::context
    end
    
    subgraph "SDK Provider Structs"
        D["internal/lambdalabs/v1/client.go"]:::minor-edit
        E["internal/nebius/v1/client.go"]:::minor-edit
    end
    
    subgraph "Core SDK"
        F["pkg/v1/client.go CloudCredential interface"]:::context
    end
    
    A --> F
    A --> D
    A --> E
    B --> A
    C --> A
    D --> F
    E --> F
    
    subgraph Legend
        L1["Major Edit"]:::major-edit
        L2["Minor Edit"]:::minor-edit  
        L3["Context/No Edit"]:::context
    end

classDef major-edit fill:#90EE90
classDef minor-edit fill:#87CEEB
classDef context fill:#FFFFFF
Loading

Notes

Testing Coverage Added:

  • LoadConfig success/failure scenarios with various file locations and formats
  • Credential parsing edge cases (missing provider, invalid provider, unknown provider)
  • Key-as-ref_id pattern with both implicit and explicit ref_id values
  • JSON/YAML marshaling round-trip testing for credential entries
  • Provider registry validation and wrapper type testing

Breaking Changes:

  • Lambda Labs provider ID changed from "lambda-labs" to "lambdalabs"
  • Added JSON tags to credential structs (should be additive, but worth verifying)

Session Info:

devin-ai-integration bot and others added 2 commits August 9, 2025 03:42
- Add cmd/bcloud directory structure with main.go and command files
- Add cobra and viper dependencies for CLI framework
- Implement bcloud <refId> <verb> command structure
- Support YAML credential configuration in ~/.bcloud/credentials.yaml
- Use YAML key as ref_id by default with optional explicit override
- Implement YAML output format for all commands
- Add core commands: create, terminate, list, get, types
- Integrate with existing CloudClient interfaces and provider implementations
- Support LambdaLabs provider with extensible factory pattern

Co-Authored-By: Alec Fong <[email protected]>
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@theFong
Copy link
Member

theFong commented Aug 9, 2025

I don't like how we are doing factory, config etc can we do something closer to this design?

package creds

import (
	"encoding/json"
	"fmt"
	"gopkg.in/yaml.v3"
)

// ---------- Core interfaces & registry ----------

type ProviderCredential interface {
	GetCloudProviderID() string
}

var providerRegistry = map[string]func() ProviderCredential{}

func RegisterProvider(id string, factory func() ProviderCredential) {
	providerRegistry[id] = factory
}

// ---------- Wrapper type (provider-discriminated union) ----------

type CredentialEntry struct {
	// Persisted "provider" key in files
	Provider string `json:"provider" yaml:"provider"`
	// In-memory typed value; excluded from default serialization
	Value ProviderCredential `json:"-" yaml:"-"`
}

// Common decode from a generic map (used by JSON & YAML)
func (c *CredentialEntry) decodeFromMap(m map[string]any) error {
	rawProv, ok := m["provider"]
	if !ok {
		return fmt.Errorf("missing 'provider'")
	}
	provider, ok := rawProv.(string)
	if !ok || provider == "" {
		return fmt.Errorf("invalid 'provider'")
	}
	factory, ok := providerRegistry[provider]
	if !ok {
		return fmt.Errorf("unknown provider: %s", provider)
	}
	cred := factory()

	// Reuse JSON to map→struct unmarshal; unknown keys are ignored by struct
	b, err := json.Marshal(m)
	if err != nil {
		return err
	}
	if err := json.Unmarshal(b, cred); err != nil {
		return err
	}

	c.Provider = provider
	c.Value = cred
	return nil
}

// Common encode to a generic map (used by JSON & YAML)
func (c CredentialEntry) encodeToMap() (map[string]any, error) {
	if c.Value == nil {
		return nil, fmt.Errorf("nil credential Value")
	}
	b, err := json.Marshal(c.Value) // serialize provider-specific fields
	if err != nil {
		return nil, err
	}
	out := map[string]any{}
	if err := json.Unmarshal(b, &out); err != nil {
		return nil, err
	}
	out["provider"] = c.Value.GetCloudProviderID()
	return out, nil
}

// ---------- JSON round-trip ----------

func (c *CredentialEntry) UnmarshalJSON(b []byte) error {
	m := map[string]any{}
	if err := json.Unmarshal(b, &m); err != nil {
		return err
	}
	return c.decodeFromMap(m)
}

func (c CredentialEntry) MarshalJSON() ([]byte, error) {
	m, err := c.encodeToMap()
	if err != nil {
		return nil, err
	}
	return json.Marshal(m)
}

// ---------- YAML round-trip ----------

func (c *CredentialEntry) UnmarshalYAML(n *yaml.Node) error {
	m := map[string]any{}
	if err := n.Decode(&m); err != nil {
		return err
	}
	return c.decodeFromMap(m)
}

func (c CredentialEntry) MarshalYAML() (interface{}, error) {
	m, err := c.encodeToMap()
	if err != nil {
		return nil, err
	}
	return m, nil // let yaml encode the map
}

// ---------- File container ----------

type CredentialFile struct {
	Credentials map[string]CredentialEntry `json:"credentials" yaml:"credentials"`
}

// ---------- Provider implementations this would be in the respective provider packages ----------

// Lambda Labs
type LambdaLabsCredential struct {
	APIKey          string `json:"api_key"           yaml:"api_key"`
	DefaultLocation string `json:"default_location"  yaml:"default_location"`
}
func (l *LambdaLabsCredential) GetCloudProviderID() string { return "lambdalabs" } // use provider id const

// Nebius
type NebiusCredential struct {
	ServiceAccountJSON string `json:"service_account_json" yaml:"service_account_json"`
	DefaultLocation    string `json:"default_location"     yaml:"default_location"`
	RefID              string `json:"ref_id,omitempty"     yaml:"ref_id,omitempty"`
}
func (n *NebiusCredential) GetCloudProviderID() string { return "nebius" } // use provider id const

// Register at init (or from each provider package's init)
func init() {
	RegisterProvider("lambdalabs", func() ProviderCredential { return &LambdaLabsCredential{} }) // use provider id const
	RegisterProvider("nebius",     func() ProviderCredential { return &NebiusCredential{} }) // use provider id const
}

@theFong
Copy link
Member

theFong commented Aug 9, 2025

lets test this method as well

…eedback

- Replace custom ProviderCredential with existing CloudCredential interface
- Add DefaultLocationProvider interface for CLI-specific functionality
- Update factory pattern to work with CloudCredential interface
- Maintain key-as-ref_id pattern with improved architecture
- All lint checks and tests pass

Co-Authored-By: Alec Fong <[email protected]>
}
cred := factory()

if _, hasRefID := m["ref_id"]; !hasRefID {
Copy link
Member

Choose a reason for hiding this comment

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

lets write a test that says all registered providers must have a ref_id in struct

Copy link
Member

Choose a reason for hiding this comment

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

we don't need to recreate the credentials here this exactly logic lies in internal/lambda/v1... lets just register them. this file can probably removed and registration can happen in config.go

Copy link
Member

Choose a reason for hiding this comment

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

i think this file can be deleted

devin-ai-integration bot and others added 2 commits August 9, 2025 23:58
…K implementations

- Delete cmd/bcloud/providers/factory.go as requested
- Remove cmd/bcloud/config/providers.go and move registration to config.go
- Use existing LambdaLabsCredential and NebiusCredential from SDK with wrapper types
- Add CLI-specific DefaultLocationProvider interface for default_location field
- Add test ensuring all registered providers have RefID field in struct
- Update all command files to work with simplified credential system
- All lint checks and tests pass

Co-Authored-By: Alec Fong <[email protected]>
…sing

- Test LoadConfig with various scenarios: success, missing files, invalid YAML
- Test credential parsing edge cases: missing provider, invalid provider, unknown provider
- Test key-as-ref_id pattern with both explicit and implicit ref_id values
- Test JSON/YAML marshaling and unmarshaling for credential entries
- Test provider registry functionality and wrapper type validation
- Test error handling for malformed configurations and nil values
- Test default settings application and file path resolution
- All tests use isolated temp directories and proper cleanup
- Fixed lint issues: error checking for os.Setenv/os.Chdir, file permissions, formatting

Co-Authored-By: Alec Fong <[email protected]>
var providerRegistry = map[string]func() v1.CloudCredential{}

var providerIDToRegistryKey = map[string]string{
"lambda-labs": "lambdalabs",
Copy link
Member

Choose a reason for hiding this comment

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

these should be the same (provider id and registry key) strings lets refactor


if _, hasRefID := m["ref_id"]; !hasRefID {
m["ref_id"] = yamlKey
if _, hasRefID := m["RefID"]; !hasRefID {
Copy link
Member

Choose a reason for hiding this comment

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

lets go in to the provider credential and use tags to change them to ref_id api_key etc.

- Add JSON tags to LambdaLabsCredential and NebiusCredential structs
  - Use lowercase field names: ref_id, api_key, service_account_key, project_id
- Unify Lambda Labs provider ID from 'lambda-labs' to 'lambdalabs' to match registry key
- Remove providerIDToRegistryKey mapping from config.go (no longer needed)
- Add provider constants (ProviderLambdaLabs, ProviderNebius) to eliminate goconst lint warnings
- Update all tests to use new lowercase field names and provider constants
- All tests pass and lint checks pass

Co-Authored-By: Alec Fong <[email protected]>
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.

1 participant