-
Notifications
You must be signed in to change notification settings - Fork 180
Server personality #413
Server personality #413
Conversation
@jrperritt could you please review? |
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.
Since it's fixed to be only path
and contents
, maybe we can have a Personality
struct:
type Personality struct {
Path string
Contents string
}
and then have the field definition in the CreateOpts
(and the like) be
Personality []Personality
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.
Good call. Perhaps Contents
could be []byte
and base64 encoded on Create and Rebuild, similar to how UserData
is base64 encoded on Create.
type Personality struct {
Path string
Contents []byte
}
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.
👍
@KevinPike Great catch. Just one suggestion above. |
I've tested this with Ubuntu and it works. However, I haven't had luck with Windows. This post uses personality with a file path I don't think it's working on Windows because in Go |
I don't know much (read: anything) about Windows environments, but maybe you can use a forward-slash instead? For example, "C:/cloud-automation/bootstrap.cmd". |
It turns out that I updated Personality to marshal using MarshalJSON. I've tested this on Ubuntu 14.04 and Windows Server 2012. |
…o personality Conflicts: openstack/compute/v2/servers/requests_test.go
Conflicts: openstack/compute/v2/servers/requests_test.go
Ready for review |
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.
What is the intended use case of the method? It seems like the base64 encoding process should go in the ToServerCreateMap
method.
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.
The file base64 encode process is used by ToServerCreateMap
and ToServerRebuildMap
. Having File
implement MarshalJSON
seemed like a good way to reduce putting the encode logic in two places and have a File
know how to marshal itself.
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.
Oh, I see; it's implementing the Marshaler
interface. Can you add a comment above File
mentioning that? Something like: "File implements the json.Marshaler
interface, so when a Create
or Rebuild
operation is requested, json.Marshal
will call File
's MarshalJSON
method." The only reason for explicitness is because I don't think we have this anywhere else. Also, could you add a Personality
field to the acceptance test that creates a server?
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.
Sure! 👍
It looks like the unit tests are failing. From Travis: |
There we go. Thank you for the heads up |
Looks good. Thank you for the PR 😄 🚀 |
This pull request changes server Personality from
[]byte
to[]map[string]string
, where each map is expected to contain path and base64 encoded contents.