-
Notifications
You must be signed in to change notification settings - Fork 152
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
feat(instance): import file for cloud-init #1525
Conversation
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.
LGTM
98dd3fb
to
53c1749
Compare
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.
LGTM
Reimplemented using ArgSpecs. |
Hmm, seems not working on user-data, will investigate tomorrow. |
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.
We should also:
- Add a least on test in the core package
- Add documentation for flag that have
CanLoadFile: true
- Handle Autocompletion (maybe in another MR , if so we need an issue)
internal/core/arg_file_content.go
Outdated
} | ||
|
||
for _, v := range fieldValues { | ||
if v.Kind() != reflect.String { |
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 about StringPointer
? is it handled in getValuesForFieldByName
?
@jtherin can you please add at least an example too 🙏 |
fieldName := strcase.ToPublicGoName(argSpec.Name) | ||
fieldValues, err := getValuesForFieldByName(reflect.ValueOf(cmdArgs), strings.Split(fieldName, ".")) | ||
if err != nil { | ||
continue |
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.
Maybe we should return an error and not fail silently ?
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.
validate.go line 48 et 78 also execute getValuesForFieldByName, log an info and continue.
It would not be too much to log the same info many times ?
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.
see coment
Add support of file import for cloud-init
$ scw instance server create image=ubuntu_focal name=test cloud-init=@/tmp/test