update connectedk8s for ARM metadata 2022-09-01 #6328
Conversation
ghost
added
ghost
requested a review
from 
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
TheOnlyWei
force-pushed
the
update-metadata-2022-09-01
branch
from
236f076
to
c1fe954
Compare
TheOnlyWei
force-pushed
the
update-metadata-2022-09-01
branch
from
c1fe954
to
e83d6be
Compare
| dp_endpoint_dogfood, release_train_dogfood = validate_env_file_dogfood(values_file, values_file_provided) | ||
| config_dp_endpoint, release_train_custom = validate_env_file_dogfood(values_file, values_file_provided) | ||
| # Get the values or endpoints required for retreiving the Helm registry URL. | ||
| if "dataplaneEndpoints" in metadata: |
There was a problem hiding this comment.
For Context, we added dataplane endpoints in ARM metadata response for the api version 2022-09-01 with this PR
https://msazure.visualstudio.com/DefaultCollection/One/_git/AzureUX-ARM/pullrequest/6838164 (internal)
There was a problem hiding this comment.
The dataplaneEndpoints values are not populated for the azure clouds at the moment, however we initialize it in our configs for the disconnected scenarios so that the correct endpoint values are returened.
In future Azure clouds could make use of these fields as well
| active_directory = metadata["authentication"]["loginEndpoint"] | ||
| cmd_helm_install.extend( | ||
| [ | ||
| "--set", "systemDefaultValues.azureResourceManagerEndpoint={}".format(resource_manager), |
There was a problem hiding this comment.
just for reviewers, These are all the Helm overrides needed for disconnecte, all of these agent overrides are already merged in upstream ClusterConfigurationAgents repo
TheOnlyWei
force-pushed
the
update-metadata-2022-09-01
branch
from
e83d6be
to
9697bdd
Compare
TheOnlyWei
changed the title
TheOnlyWei
force-pushed
the
update-metadata-2022-09-01
branch
3 times, most recently
from
e95f1cf
to
04d67a2
Compare
TheOnlyWei
changed the title
|
Could you please add some tests for this PR change? |
|
Hi @zhoxing-ms the changes are for an on-premise environment and the tests only have access to Azure Cloud, so it is currently not possible to test the new endpoints without the on-premise environment. We already have internal tests for the on-premise environment that uses this updated code. |
|
@akashkeshari Could you please help review this PR again? |
.gitignore
Outdated
| # Ignore test data files | ||
| **/connectedk8s/azext_connectedk8s/tests/latest/data/ | ||
|
|
There was a problem hiding this comment.
Why do you need to modify this file?
There was a problem hiding this comment.
Otherwise, you will commit a bunch of test files if you run the Azure Cloud tests.
There was a problem hiding this comment.
In fact, we do upload some files used for testing into /connectedk8s/azext_connectedk8s/tests/latest/data/, such as https://github.com/Azure/azure-cli/tree/dev/src/azure-cli/azure/cli/command_modules/resource/tests/latest/data
There was a problem hiding this comment.
Those files I mentioned are autogenerated intermediate files from the test and were not committed before this PR, so I didn't see a reason to commit it for this PR. I don't think those files are supposed to be committed, but there was no gitignore option, so I decided to add it as a fix.
There was a problem hiding this comment.
OK, got it. Please add this line to the end of the file instead of the beginning.
# Ignore test data files
In addition, please provide a more detailed explanation in the comment
There was a problem hiding this comment.
I can move it to the end of the file, but is a more detailed description required? I feel like you're supposed to have generalized descriptions so other teams can put their own test data under the same gitignore comment.
There was a problem hiding this comment.
In fact, the files in the xxx/tests/latest/data/ directory of other module are useful files that will be used for testing, they need to be added to the source code. Therefore, the ignore logic of connectedk8s module is unique at present, so I suggest making a more detailed explanation
There was a problem hiding this comment.
Okay, it seems the tests are not supposed to be using tests/latest/data to dump one-time-use test output. I will remove the git ignore.
TheOnlyWei
force-pushed
the
update-metadata-2022-09-01
branch
from
04d67a2
to
9facf54
Compare
️✔️Azure CLI Extensions Breaking Change Test
|
|
[Release] Update index.json for extension [ connectedk8s ] : https://dev.azure.com/azclitools/internal/_build/results?buildId=79071&view=results |
This PR adds ability to fetch new properties from 2022-09-01 ARM metadata in an on-premises environment that is currently in preview.
Related command
az connectedk8s
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally?IMPORTANT: I ran "azdev style azext_connectedk8s", but there are preexisting formatting errors unrelated to my change. I assumption is the project never followed the style guides to begin with. If the style guide should be followed, I am not going to fix those issues in this PR as there are many of them and likely not related to my changes.
For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update
src/index.jsonautomatically.You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify
src/index.json.