-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
New resource: DevSpace Controller for AKS cluster #2086
Conversation
Introduce pacakge for devspace controller.
…or DevSpaces controll… Add code to support creation of DevSpaces controller.
Implemented RUD operations for devspaces controller.
…ontroller. Add doc and test for DevSpace controller, and refine the schema.
Sku is set to be required and update doc.
For styling issue for devspace controller.
Update tests and styling code for DevSpace controller.
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.
Hi @metacpp,
Thanks for the PR. I've left some mostly minor comments inline but it mostly LGTM,
} | ||
|
||
var result devspaces.Controller | ||
if result, err = future.Result(client); err != 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.
Could we use client.Get()
here instead like the majority of other resources?
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.
Is this the new pattern for async
API ?
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 wouldn't call it new, most resources are coded that way. There are only 3 places in the entire provider where future.result()
is called
also, the provider is now on |
Co-Authored-By: metacpp <[email protected]>
…rovider-azurerm into devspace
…re function to fetch result.
@katbyte The PR is updated by resolving all issues. |
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.
@metacpp, Thank you for the updates. LGTM 👍
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.
left a few comments inline but this otherwise LGTM 👍
@tombuildsstuff I followed up your suggestions in #2182 |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
This PR is a follow up on #1488 to add DevSpace controller resource for AKS cluster.