Refactor how to create new orka provider to have 1 way to create a new provider
The following discussion from !122 (merged) should be addressed:
-
@steveazz started a discussion: (+6 comments) question: I'm curious why we moved this into its own method. Couldn't this cause future changes to call
newWithConfig
instead ofNew
? Shouldn'tNew
be the only thing that creates a new Executor instance?Shouldn't all this logic be inside of
New
? New isn't really do anything else and just acting as a proxy method which I can't see much benefit. Maybe a better name for this would bevalidateSSH
and don't have it return a new object?
since we are using some parsing functions to both validate the inputs and generate an orka client (see https://gitlab.com/gitlab-org/ci-cd/custom-executor-drivers/autoscaler/-/blob/master/providers/orka/provider.go#L52-83), so this method doesn't work unless we parse twice
Good point! Maybe we can have something like below? But let's create a follow-up issue here not to block this merge request anymore
providers/orka/provider.go#New
func New(cfg globalConfig.Global, logger logging.Logger) (providers.Provider, error) {
err := validateConfig(cfg.Orka)
if err != nil {
return nil, fmt.Errorf("invalid Orka configuration: %w", err)
}
endpoint, err := newEndpointURL(cfg.Orka.Endpoint)
if err != nil {
// TODO: Maybe create custom error type here so we don't duplicate
return nil, fmt.Errorf("invalid Orka configuration: %w", err)
}
timeout := time.Duration(cfg.Orka.Timeout)
if timeout == 0 {
timeout = time.Minute
}
return &Provider{
config: cfg.Orka,
logger: logger,
client: orka.New(
endpoint,
cfg.Orka.Token,
&orka.Config{
Logger: logger,
BackoffSettings: nil,
Client: &http.Client{
Timeout: timeout,
},
},
),
}, nil
}
func validateConfig(config config.Provider) error {
if config.Endpoint == "" {
return errors.New("Endpoint is required")
}
if config.Token == "" {
return errors.New("Token is required")
}
if config.BaseImage == "" {
return errors.New("BaseImage is required")
}
switch config.Cores {
case 3, 4, 6, 8, 12, 24:
break
default:
return errors.New("Cores must be 3, 4, 6, 8, 12, or 24")
}
return nil
}
func newEndpointURL(endpoint string) (*url.URL, error) {
endpointURL, err := url.Parse(endpoint)
if err != nil {
return nil, fmt.Errorf("Endpoint URL is not valid: %w", err)
}
if endpointURL.Scheme != "http" {
return nil, errors.New("Endpoint should start with http://")
}
return endpointURL, nil
}