Skip to content
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

K8s extension/release 1.3.6 #5424

Merged

Conversation

deeksha345
Copy link
Contributor


This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

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.
The precondition is to put your code inside this repository and upgrade the version in the pull request but do not modify src/index.json.

Miraj50 and others added 18 commits September 29, 2022 13:17
* First draft for Data Protection K8s backup extension (Pending internal review)

* Removing tracing

* Minor changes to improve azdev style

* Internal PR review feedback

Co-authored-by: Rishabh Raj <[email protected]>
…ad of workspace (#175)

* fix: Update DCR creation to Clusters resource group instead of workspace

* .

* .

* casing check
adding the api version to the operation definition in the client factory
…tests

adding tests for all 4 extension types calls
@yonzhan
Copy link
Collaborator

yonzhan commented Oct 6, 2022

K8s extension

@wangzelin007
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@deeksha345
Copy link
Contributor Author

All checks are passing can I please get this PR approved and merged in, thanks!

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Oct 9, 2022

Could you add some test cases for those new features?

@deeksha345
Copy link
Contributor Author

Could you add some test cases for those new features?

Which features are you talking about? I already have test cases for the extension type calls.

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Oct 11, 2022

Which features are you talking about? I already have test cases for the extension type calls

@deeksha345 This PR does not seem to contain tests. Could you show me the code link of tests?

Fix the TypeError: cf_k8s_extension() takes 1 positional argument but 2 were given while running all az k8s-extension extension-types commands

I believe that such problems should be covered by tests

@deeksha345
Copy link
Contributor Author

I have added tests as part of our custom pipeline in our forked repo and any changes we push to upstream repo are only after all those checks pass. Those files are deleted and not included in our PRs to the upstream repo though.
Here is the link to the tests in our repo: https://github.com/AzureArcForKubernetes/azure-cli-extensions/blob/k8s-extension/public/testing/test/extensions/public/ExtensionTypes.Tests.ps1

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Oct 12, 2022

@deeksha345 Could you add scenario tests in azure-cli-extensions repo? In this way, the CI pipeline of CLI extension repo can also help you perform regression testing when you are submitting PRs

@deeksha345
Copy link
Contributor Author

We have tests in our forked repo and Bavneet said that is satisfactory for providing test coverage overall. We do not update the tests in the azure cli repo.
If I added tests also in azure-cli-extensions also I would just be adding the same tests twice and I do not see the point in that as our custom pipeline already provides the test coverage needed.

@deeksha345
Copy link
Contributor Author

Ok this is what Narayan said - These tests will be used during Official CLI releases and in their Nightly tests, which do not use our pipeline. So, yes, you will have to create tests. For our scenarios, since we require a K8s cluster to be created, and since it is not possible to create that in their runs we record these tests once and mark them such that they are just replayed and not run. All our existing tests are marked this way. Refer to the Az CLI Authoring documentation on how to mark tests for replay only.

@zhoxing-ms Can you point me to the documentation that talks about how to mark tests as replay only as I cannot find any information about it.

@zhoxing-ms
Copy link
Contributor

We have tests in our forked repo and Bavneet said that is satisfactory for providing test coverage overall. We do not update the tests in the azure cli repo.
If I added tests also in azure-cli-extensions also I would just be adding the same tests twice and I do not see the point in that as our custom pipeline already provides the test coverage needed.

@deeksha345 This is convenient for us to maintain test cases with the source code, and we can find problems as soon as possible when submitting PR. If problems are found in the tests, the PR merge can be blocked in time

@zhoxing-ms
Copy link
Contributor

@deeksha345 Please refer to this doc authoring_tests.md to write tests

@deeksha345
Copy link
Contributor Author

We have tests in our forked repo and Bavneet said that is satisfactory for providing test coverage overall. We do not update the tests in the azure cli repo.
If I added tests also in azure-cli-extensions also I would just be adding the same tests twice and I do not see the point in that as our custom pipeline already provides the test coverage needed.

@deeksha345 This is convenient for us to maintain test cases with the source code, and we can find problems as soon as possible when submitting PR. If problems are found in the tests, the PR merge can be blocked in time

@zhoxing-ms I have added the tests please take a look and merge this PR if everything looks good!

@zhoxing-ms zhoxing-ms merged commit 6ee229c into Azure:main Oct 20, 2022
@azclibot
Copy link
Collaborator

[Release] Update index.json for extension [ k8s-extension ] : https://dev.azure.com/azclitools/internal/_build/results?buildId=10042&view=results

@deeksha345
Copy link
Contributor Author

For 2 of these test scenarios - the show and the list with cluster - we require a K8s cluster to be created, and since it is not possible to create that in your runs I marked these tests as record only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants