Skip to content
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

Added server {set,get}-user-data subcommand #105

Merged
merged 2 commits into from
Sep 2, 2020
Merged

Added server {set,get}-user-data subcommand #105

merged 2 commits into from
Sep 2, 2020

Conversation

NickCao
Copy link
Contributor

@NickCao NickCao commented Aug 16, 2020

Description

Related Issues

Checklist:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you linted your code locally prior to submission?
  • Have you successfully ran tests with your changes locally?

@ddymko ddymko self-requested a review August 16, 2020 01:23
Copy link
Contributor

@ddymko ddymko left a comment

Choose a reason for hiding this comment

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

Thanks for submitting a PR there are two changes that do need to be made before I can merge this in

cmd/server.go Outdated
Short: "Set the user-data of a server",
Args: func(cmd *cobra.Command, args []string) error {
if len(args) < 2 {
return errors.New("please provide a instanceID and userData")
Copy link
Contributor

Choose a reason for hiding this comment

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

The user-data should be a flag that is passed in so it follows the same consistency as other calls.

Label is a good example to follow
https://github.com/vultr/vultr-cli/blob/master/cmd/server.go#L43
https://github.com/vultr/vultr-cli/blob/master/cmd/server.go#L342

cmd/server.go Outdated
os.Exit(1)
}

_, _ = os.Stdout.Write(rawData) // For proper handling of binary user-data
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have its own function in printer/server.go

https://github.com/vultr/vultr-cli/blob/master/cmd/printer/server.go

@NickCao
Copy link
Contributor Author

NickCao commented Aug 17, 2020

User-data can be quite lengthy and it doesn't make much sense specifying it as a param or flag. (Actually, I tried to replicate existing code in handling the user-data of baremetal servers in this PR in terms of consistency.) The same for the printer part. Maybe a better design is to just read user-data from stdin or a file, which is specified as a flag?

@ddymko
Copy link
Contributor

ddymko commented Aug 17, 2020

I didn't notice how it is currently being done in bare-metal so we can leave it as STDIN.

As for the printer I will take another look - baremetal is using https://github.com/vultr/vultr-cli/blob/master/cmd/printer/userData.go to decode the data but it may be good to revisit this behavior and change it for the better

@NickCao
Copy link
Contributor Author

NickCao commented Aug 17, 2020

There's some inherent inconsistency in the govultr library itself. For SetUserData part, it takes raw input and encodes it, however for the GetUserData part it returns the encoded data as is. Moreover it asumes that user-data is of string type, which is not necessarily true.

@ddymko
Copy link
Contributor

ddymko commented Aug 19, 2020

Agreed - the SetUserData should take in a already encoded b64 value instead of raw input and encoding it.

We will make this change in GoVultr so that the behavior is more streamlined and consistent.

@ddymko
Copy link
Contributor

ddymko commented Aug 27, 2020

@NickCao We released GoVultr 0.5.0 which changes the setUserData call to take in a string value (which should be base64 encoded) and hands it off to the API which is then validated it is a proper b64.

https://github.com/vultr/govultr/releases/tag/v0.5.0

@NickCao
Copy link
Contributor Author

NickCao commented Aug 28, 2020

I moved the userdata part to it's own subcommand, and added a flag to specify the userdata (to read).
Still not satisfied with the printer part though, as the output might be piped for further processing and the column header is just annoying. But for consistency, leave it as is can be a choice.

@ddymko ddymko merged commit 3d82cd4 into vultr:master Sep 2, 2020
@ddymko ddymko added this to the v0.4.0 milestone Sep 2, 2020
@mamclaughlin mamclaughlin mentioned this pull request Sep 3, 2020
@acyment
Copy link

acyment commented Jul 12, 2023

It'd be great to be able to refer to a file in order to set user-data, as mentioned in one of the initial comments

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.

None yet

3 participants