-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bunch o stuff #26
Bunch o stuff #26
Conversation
Dev latest
fix ssh user code to send correct params add missing params to ssh key code
Add fixes to random api return types
Latestwork
Hey @yakmoose, thanks for the PR! We'll take a look at your changes and get back to you soon :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for being patient with this.
I only have a few requested changes and questions as everything else looks pretty good. Once those are resolved, this should be ready to be merged.
"context" | ||
) | ||
|
||
// ListIPAddresses fetchs a list of IP addresses and subnets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// ListIPAddresses fetchs a list of IP addresses and subnets. | |
// ListIPAddresses fetches a list of IP addresses and subnets. |
pkg/models/job.go
Outdated
@@ -1,6 +1,7 @@ | |||
package models | |||
|
|||
type ( | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/utils/utils.go
Outdated
s := buf.String() | ||
|
||
return s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s := buf.String() | |
return s | |
return buf.String() |
import "github.com/sitehostnz/gosh/pkg/models" | ||
|
||
type ( | ||
// ListResponse represents the return from the /cloud/stack/images/list_all.json endpoint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// ListResponse represents the return from the /cloud/stack/images/list_all.json endpoint. | |
// ListResponse represents the return from the /cloud/stack/image/list_all.json endpoint. |
pkg/api/cloud/ssh/user/delete.go
Outdated
"github.com/sitehostnz/gosh/pkg/utils" | ||
) | ||
|
||
// Delete deletes a cloud database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Delete deletes a cloud database. | |
// Delete an existing SSH user. |
pkg/api/cloud/ssh/user/list.go
Outdated
"github.com/sitehostnz/gosh/pkg/utils" | ||
) | ||
|
||
// List returns a list of stack images, specific to the customer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// List returns a list of stack images, specific to the customer. | |
// Get a list of all SSH users. |
pkg/api/ssh/key/response.go
Outdated
models.APIResponse | ||
} | ||
|
||
// DeleteResponse represents a result of the delete an SSH Key call. | ||
// DeleteResponse represents a result of a delete an SSH Key call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// DeleteResponse represents a result of a delete an SSH Key call. | |
// DeleteResponse represents the result of deleting an SSH key. |
pkg/api/dns/recordget.go
Outdated
|
||
// GetRecordWithRecord is a special case, for mainly when we are creating records where we need to get back what we just created. | ||
func (s *Client) GetRecordWithRecord(ctx context.Context, request models.DNSRecord) (response models.DNSRecord, err error) { | ||
records, err := s.ListRecords(ctx, ListRecordsRequest{Domain: request.Domain}) | ||
if err != nil { | ||
return response, err | ||
} | ||
|
||
for _, r := range records.Return { | ||
if r.Name == request.Name && | ||
r.Type == request.Type && | ||
r.Content == request.Content && | ||
(r.Priority == request.Priority || r.Priority == "0") { | ||
return r, nil | ||
} | ||
} | ||
return response, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is currently used by terraform-provider-sitehost
. Do you have a branch of the Terraform provider that no longer needs this?
Either way, it should remain here while its still used in the main branch of terraform-provider-sitehost
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an api change at your end, that returned the id of the created record, meaning we didn't need this to find what we just made. I'll get a PR for the provider sorted shortly.
const ( | ||
// SchedulerType is a job that runs on a scheduler, I am sure someone can fill this out more. | ||
SchedulerType string = "scheduler" | ||
|
||
// DaemonType is a job that runs on a daemon, I am sure someone can fill this out more. | ||
DaemonType = "daemon" | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These aren't used anywhere in the code base. Are they used in your Terraform provider work? If not, then they should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are used in the terraform provider, so we can determine what type of task we are waiting for. Felt like it belonged here since it is something described by the API.
|
||
// GetResponse returns a single image from the /cloud/stack/images/get.json endpoint. | ||
GetResponse struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used in the code base either, so it should also be removed if there isn't a reason for it being here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will yank this for now, I think it was something I was starting to flesh out in the TF provider.
Review fixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is now ready to merged.
We look forwards to seeing your Terraform provider work when its ready :)
Adds in a chunk of cloud stack stuff which I've been dog fooding in my terraform work, which will come in another PR.