provider/do: add support for Digital Ocean - #9187
Conversation
|
🎉 |
| return nil, err | ||
| } | ||
|
|
||
| if err := waitForAction(ctx, m.client, action); err != nil { |
There was a problem hiding this comment.
Isn't it better to use waitstate package to wait for machinestate.Running state? It is used in AWS provider here and seems like it was written exactly for this job. Moreover, I think that it's better to have consistent logic across all providers for starting/stopping machines.
| debug := false | ||
| if debug { | ||
| start := time.Now() | ||
| log.Println("waiting for action to finish: ", action.ID) |
There was a problem hiding this comment.
I would rather we handled all the logging in dependency injected Logger. wdyt?
| if action.CompletedAt != nil || action.Status == actionCompleted { | ||
| return nil | ||
| } | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
Just a question: do we have any context cancellation logic?
There was a problem hiding this comment.
No we don't, moreover contexts are used wrongly - for storing global-scoped data. I'm in ongoing effort to have that all fixed.
| t.Run(test.name, func(t *testing.T) { | ||
| dropletID := 12345 | ||
| if test.dropletID != 0 { | ||
| dropletID = 0 |
There was a problem hiding this comment.
shouldn't be dropletID = test.dropletID ?
| "droplet_id": strconv.Itoa(dropletID), | ||
| }, | ||
| }), | ||
| Provider: "do", |
There was a problem hiding this comment.
i was not sure if this field is used to create terraform template where they use digitalocean as a prefix link.
| return errors.New("size cannot be empty") | ||
| } | ||
|
|
||
| if m.Region == "" { |
There was a problem hiding this comment.
Shouldn't this check be implemented by Region type itself?
| // isValid checks whether the given region is valid or not | ||
| func (r Region) isValid() bool { | ||
| for _, validRegion := range validRegions { | ||
| if validRegion == string(r) { |
There was a problem hiding this comment.
Not a bug but var validRegion = []Region{ /* .... */ } will drop this type casting.
| import "testing" | ||
|
|
||
| func TestCredential_Valid(t *testing.T) { | ||
| cred := &Credential{ |
There was a problem hiding this comment.
Another (small) note: all tests in this file could be merged into one with table style fashion.
| droplet["ssh_keys"] = []int{keyID} | ||
| } else if keyIds, ok := s.([]int); ok { | ||
| keys := []int{keyID} | ||
| if len(keyIds) != 0 { |
There was a problem hiding this comment.
even var keyIds []int = nil can be expanded keyIds... so no need for this check.
| labels := []string{dropletName} | ||
| if count > 1 { | ||
| for i := 0; i < count; i++ { | ||
| labels = append(labels, fmt.Sprintf("%s.%d", dropletName, i)) |
There was a problem hiding this comment.
hmm if count = 2 then we will have labels = []string{"droplet", "droplet.0", "droplet.1"}. Doesn't seem right.
4 participants
No description provided.