Fix az aks get-credentials on Windows
#4762
Conversation
|
Should update HISTORY.rst with description of fix. |
mboersma
force-pushed
the
fix-windows-get-creds
branch
from
572d716 to
8d055d5
Compare
|
@mboersma There's a conflict after a prev. PR merge. |
| @@ -1295,13 +1295,17 @@ def aks_get_credentials(client, resource_group_name, name, admin=False, | |||
| pass | |||
|
|
|||
| # merge the new kubeconfig into the existing one | |||
| with tempfile.NamedTemporaryFile(mode='w+t') as additional_file: | |||
| fd, temp_path = tempfile.mkstemp() | |||
| additional_file = os.fdopen(fd, 'w+t') | |||
There was a problem hiding this comment.
t is not necessary as text mode is the default, but if you prefer explicit, that is fine
| logger.warning('Failed to merge credentials to kube config file: %s', ex) | ||
| finally: | ||
| additional_file.close() | ||
| os.remove(temp_path) |
There was a problem hiding this comment.
I suggest you add a couple of tests for this logic. The related issue impacts quite a few users, so good to build up regression tests
There was a problem hiding this comment.
Also, a bit straightforward option is to pass the content to merge_kubernetes_configurations rather build up a temp file and gets read out again. Of course this requires some refactor on old code, so up to you.
|
I'll write some unit tests and rebase this. |
mboersma
changed the title
az aks get-credentials on Windowsaz aks get-credentials on Windows
|
@mboersma FYI code freeze for next release is 11/8. |
mboersma
force-pushed
the
fix-windows-get-creds
branch
from
8d055d5 to
11fea79
Compare
|
@mboersma is this PR still work in progress? |
|
@troydai I want to add tests, but I realized there are already good unit tests around the underlying But AKS has been so popular we're fighting capacity issues right now, and I've been unable to re-run the scenario test despite trying in the background all day long: So I'd really like to see this merged because I know it fixes an issue on Windows, but I'm having trouble adding a |
mboersma
changed the title
az aks get-credentials on Windowsaz aks get-credentials on Windows
|
Okie Dokie. I will merge this PR. I'm interested to know the capacity issue since we'd like to run automation against live environment routinely as well. |
|
When will this be shipped? |
2 similar comments
|
When will this be shipped? |
|
When will this be shipped? |
|
I will be released on 11/15. |
vmss: support basic tier of vms (Azure#4847) Azure IoT CA functionality (Azure#4804) * Adds support for X.509 Certificates in IoT Hub. * Modifies release notes and changes help link. * Cleans up .gitignore, and HISTORY.rst. Passes a single factory in commands.py. Moves test utilities. Adds a file completer. * Updates style after rebase based on additional linter restrictions. Reserved Instance cli public PR (Azure#4838) Handle dashes in extension names better (Azure#4839) Fix `az aks get-credentials` on Windows (Azure#4762) * Fix `az aks get-credentials` on Windows * Move k8s file merge logic to helper function appservice: support assign managed service identity to webapp/functionapp (Azure#4837) Extension examples small improvement (Azure#4852) Fixing appservice list-locations (Azure#4846) Add extension name to telemetry and UA header (Azure#4854) Fix Azure#4825. (Azure#4853) Extensions `az extension add` --yes param as flag (Azure#4858) Fix issue Azure#2752. (Azure#4859)
NamedTemporaryFilecan't be opened more than once on Windows, so this usesmkstemp. Tested interactively on Windows 10, macOS, and Ubuntu Linux.