-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
update connectedk8s for ARM metadata 2022-09-01 #6328
Conversation
Thank you for your contribution! We will review the pull request and get back to you soon. |
236f076
to
c1fe954
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
minor comments
active_directory = metadata["authentication"]["loginEndpoint"] | ||
cmd_helm_install.extend( | ||
[ | ||
"--set", "systemDefaultValues.azureResourceManagerEndpoint={}".format(resource_manager), |
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.
just for reviewers, These are all the Helm overrides needed for disconnecte, all of these agent overrides are already merged in upstream ClusterConfigurationAgents repo
e83d6be
to
9697bdd
Compare
e95f1cf
to
04d67a2
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to modify this file?
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.
Otherwise, you will commit a bunch of test files if you run the Azure Cloud tests.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 azdev
required)python scripts/ci/test_index.py -q
locally?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.json
automatically.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
.