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

Use remote Kustomize base for deploying AKS Periscope #4904

Merged
merged 6 commits into from
Jun 2, 2022

Conversation

peterbom
Copy link
Contributor

@peterbom peterbom commented May 26, 2022

The latest Periscope release provides a Kustomize base file for deploying Periscope with Windows support.

These changes make use of Kustomize to remove the need for magic string replacement, and minimize the required changes to az-cli when new Periscope versions are released (we'll only have to update the release version and image tag, not the entire deployment yaml).

NOTE: the minimum supported kubectl version for Kustomize is 1.14 (released March 2019).

cc. @Tatsinnit, @FumingZhang


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.

@ghost ghost requested review from zhoxing-ms and wangzelin007 May 26, 2022 23:24
@ghost ghost assigned zhoxing-ms May 26, 2022
@ghost ghost added this to the Jun 2022 (2022-07-05) milestone May 26, 2022
@ghost ghost added the Auto-Assign Auto assign by bot label May 26, 2022
@ghost ghost requested a review from yonzhan May 26, 2022 23:24
@ghost ghost added the AKS label May 26, 2022
@yonzhan
Copy link
Collaborator

yonzhan commented May 26, 2022

AKS

Copy link
Member

@Tatsinnit Tatsinnit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☕️💡 LGTM , as long as it’s tested, which is the case 🙏❤️

Copy link
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented May 27, 2022

Could you add some tests for these change?

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented May 27, 2022

Please add the description in history notes HISTORY.rst. If you need to upgrade the extension version to release, please also modify the version of setup.py

@peterbom
Copy link
Contributor Author

Could you add some tests for these change?

I'm attempting to add a test now, but running into the issue that the command is interactive (there are a couple of prompts: one to confirm the user consents to the operation, and one to find out whether the user wishes to see the analysis results).

The common approach to avoid this seems to be to add a --yes ("do not prompt for confirmation") parameter, but:

  • So far I can't quite understand the code required to do this (like why some parameters are defined in _params.py and some in _help.py.
  • This particular command has two different prompts, and it's unclear what the default should be for the second.

All in all, this change seems to entail a non-trivial bit of design work that's not relevant to the intent of the original PR.

Should we address this (and add the test) in a separate PR?

@zhoxing-ms, @FumingZhang, @Tatsinnit for thoughts.

@zhoxing-ms
Copy link
Contributor

All in all, this change seems to entail a non-trivial bit of design work that's not relevant to the intent of the original PR.
Should we address this (and add the test) in a separate PR?

LGTM

@FumingZhang
Copy link
Member

Yeah, would appreciate if you could add some unit test for these commands, since they are not related to ARM resource, I suppose live/e2e test based on vcr/recording is not suitable.

@peterbom
Copy link
Contributor Author

Yeah, would appreciate if you could add some unit test for these commands, since they are not related to ARM resource, I suppose live/e2e test based on vcr/recording is not suitable.

Discussed with @FumingZhang offline: we will work on one or two follow-up PRs to shuffle the code around (modelled on the recent draft additions) to make it more amenable to unit tests.

Further discussion may be required on whether the 'live' tests are necessary/appropriate.

@peterbom
Copy link
Contributor Author

peterbom commented Jun 2, 2022

@zhoxing-ms, @FumingZhang: Added a live test to cover the kollect command.

@peterbom peterbom force-pushed the feature/update-periscope branch from d0e780c to dea2434 Compare June 2, 2022 02:04
@FumingZhang
Copy link
Member

The CI failed due to changes in the behavior of the rp and will be fixed in another PR #4919.

Queued a pipeline to test the newly added test case.

@zhoxing-ms zhoxing-ms merged commit aa1d0b4 into Azure:main Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AKS Auto-Assign Auto assign by bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants