-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Add Kubernetes support to ACS. #1258
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
yugangw-msft
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.
Please fix pylint errors and get CI green. Also clarify review comments.
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.
please delete this line, as this is a duplicate of 3 lines above
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.
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.
Consider to exposing the version like you did for dcos. Up to you at this moment though
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.
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.
use sys.stdout.write just be consistent?
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 prefer print for it's platform indepdent newline.
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.
mgmt_acs should be moved into this command module rather than the acs module having a dependency on vm.
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.
@derekbekoe, this will be cleaned up in 2 weeks per conversation with Brendan
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.
ack. I promise to fix this asap, but I need to get this in for the 11/11 cut.
Thanks for the flexibility.
yugangw-msft
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.
More pylint errors to clean up
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.
this change is unnecessary and might well cause a pylint errir
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.
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.
For your consideration, azure-cli commands use password terminology across for this concept. For consistency I would suggest you use the password here. Both terminologies are correct, but have used at different layers.
At this point it is your call, because you have to update related method as well
d9500f1 to
7cb69e5
Compare
|
@brendandburns , other than pylint error, you should take a look at the test failure of |
7cb69e5 to
c2b8ba2
Compare
|
@yugangw-msft I actually see that output in the example output that the JMESPath assertion is testing. I'm really not certain why the query isn't picking it up. Is there a quick way to just run that one test? Thanks |
|
|
|
@brendandburns, yes you can run that test: python -m unittest azure.cli.command_modules.vm.tests.test_vm_commands.AzureContainerServiceScenarioTest.test_acs_create_update and to fix the test (assume you have communicated with @sauryadas), is to use fully qualified property path such as |
|
@brendandburns, |
1a41ffd to
de94d0e
Compare
|
PR #1059 has been merged so a rebase will be required with a couple of changes to the acs/commands.py and vm/commands.py files. Changes required in
Would change to: Changes required for the
|
de94d0e to
1182d91
Compare
|
@derekbekoe yep, I made that refactor, ptal. thanks! |
aeb5496 to
1038577
Compare
1038577 to
fe3d4bc
Compare
@yugang-msft
Thanks!
--brendan