-
Notifications
You must be signed in to change notification settings - Fork 18
Add user_data field to instance create
#1898
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This looks good to me functionally. The tooltip/help text could use workshopping.
For one thing, if we're going to have help text anyway, I don't think we need the tooltip. The whole section is already collapsed under More importantly, though, we should explain better what this content is supposed to be (the API doc this is based on could use the same improvement). As discussed in chat, it would make things a lot easier if we could be specific an say "this is about cloud-init" rather than "instance initialization systems (such as cloud-init)". Just so I'm not complaining without making a constructive suggestion, here is some text that is IMO at least a marginal improvement on what's there:
In lieu of our own doc explaining this at length, this at least gives them something. I'm on the fence about whether to mention that the content should be plain text. |
|
Thanks for the feedback! Added the links object and test that checks that the external links are accessible – I could pull that out into its own PR if that's helpful. Its become a bit of a mishmash because I've changed |
app/util/links.ts
Outdated
| external, | ||
| } | ||
|
|
||
| export default links |
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.
I should put in a lint rule or something about this — I think the best practice is to avoid default exports. Bundlers don't like them, something like that. They don't really do anything useful anyway. For now you can also collapse this down, we don't need to distinguish external. I'd just do export const links = { cloudInitFormat: ... }.
|
Decided the fallback to GET is overkill since the 2 links we have both work with HEAD and it's possible other ones will for the foreseeable future. |
david-crespo
left a comment
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.
I think we're good! Nice work!
oxidecomputer/console@644a45b...b9013a3 * [b9013a33](oxidecomputer/console@b9013a33) oxidecomputer/console#1898 * [fb9e9ca5](oxidecomputer/console@fb9e9ca5) oxidecomputer/console#1904 * [9ae29498](oxidecomputer/console@9ae29498) oxidecomputer/console#1897 * [6b894ceb](oxidecomputer/console@6b894ceb) oxidecomputer/console#1901 * [d80d2e7c](oxidecomputer/console@d80d2e7c) oxidecomputer/console#1886 * [2a7da0fa](oxidecomputer/console@2a7da0fa) bump vite for security fix * [700e2700](oxidecomputer/console@700e2700) oxidecomputer/console#1893
oxidecomputer/console@644a45b...b9013a3 * [b9013a33](oxidecomputer/console@b9013a33) oxidecomputer/console#1898 * [fb9e9ca5](oxidecomputer/console@fb9e9ca5) oxidecomputer/console#1904 * [9ae29498](oxidecomputer/console@9ae29498) oxidecomputer/console#1897 * [6b894ceb](oxidecomputer/console@6b894ceb) oxidecomputer/console#1901 * [d80d2e7c](oxidecomputer/console@d80d2e7c) oxidecomputer/console#1886 * [2a7da0fa](oxidecomputer/console@2a7da0fa) bump vite for security fix * [700e2700](oxidecomputer/console@700e2700) oxidecomputer/console#1893

Fixes #800 as per: #800 (comment)
Works fine with the mock API, we should test against dogfood.
We're just reading their file as a string and converting into a base64 string using the same mechanism as cert upload. Do we want to constrain the files a user can upload?