-
Notifications
You must be signed in to change notification settings - Fork 8
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
config: Support importing config using Azure API #39
Conversation
f3989d9
to
a9c816f
Compare
c084c94
to
c2968c0
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.
Looks great. I tested and it generated indeed the same file. I only have some concerns about when the Azure API mechanism is used.
c2968c0
to
4391143
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.
Some nits but LGTM!
cmd/utils/utils.go
Outdated
"github.com/briandowns/spinner" | ||
) | ||
|
||
var DefaultSpinner = spinner.New(spinner.CharSets[9], 50*time.Millisecond) |
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.
It is a lot slower in az
😅
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 agree I got bit aggressive with that 🙈 seems like they are using 200 ms frequency so I will switch to the same!
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.
Done. Feels much better! 😄
4391143
to
bf9e028
Compare
Thanks of the reviews! I will merge this one! |
I just realized that we didn't update the documentation with this alternative for |
Yeah, I opened a follow up PR with other improvement: https://github.com/Azure/kubectl-aks/pull/41/files I see that PR title isn't reflecting that 😄 |
This change introduces a fallback mechanism to import config in case Kubernetes API isn't available.
Fixes: #37
IAM changes
Since we will using Azure API to fetch config information, we will be needing new permission actions in subscription the cluster is running:
current:
Microsoft.Compute/virtualMachineScaleSets/virtualmachines/runCommand/action
new
Microsoft.Compute/virtualMachineScaleSets/virtualmachines/runCommand/action
Microsoft.Compute/virtualMachineScaleSets/virtualMachines/read
Microsoft.Compute/virtualMachineScaleSets/read
Testing done
Added an integration tests and: