Skip to content

Conversation

@norshtein
Copy link
Member


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

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 PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

@azuresdkci
Copy link

If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR:

docker run -it mcr.microsoft.com/azure-cli:latest
export EXT=<NAME>
pip install --upgrade --target ~/.azure/cliextensions/$EXT "git+https://github.com/norshtein/azure-cli-extensions.git@tosi/aks-spn-to-msi#subdirectory=src/$EXT&egg=$EXT"

@yungezz yungezz added the AKS label Oct 26, 2020
@Azure Azure deleted a comment from yonzhan Oct 26, 2020
@norshtein
Copy link
Member Author

The job "Verify Extensions Index" failed with error:

======================================================================
FAIL: test_metadata (__main__.TestIndex)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./scripts/ci/test_index.py", line 139, in test_metadata
    self.whl_cache_dir, self.whl_cache)
  File "/home/vsts/work/1/s/scripts/ci/util.py", line 86, in get_whl_from_url
    assert r.status_code == 200, "Request to {} failed with {}".format(url, r.status_code)
AssertionError: Request to https://azurecliprod.blob.core.windows.net/cli-extensions/synapse-0.1.0-py2.py3-none-any.whl failed with 500

The extension the error referencing to is relevant to aks-preview, and I've run python scripts/ci/test_index.py -q locally and succeeded:

$ python scripts/ci/test_index.py -q
----------------------------------------------------------------------
Ran 9 tests in 0.014s

OK (skipped=2)

Anything I can do to fix the CI error?

@arrownj
Copy link
Contributor

arrownj commented Oct 27, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@arrownj arrownj left a comment

Choose a reason for hiding this comment

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

Hi @norshtein , could you please also help add tests for these new parameters ?

@norshtein
Copy link
Member Author

Hi @arrownj , could you add a "Do-Not-Merge" label for this PR? AKS have a server-side behavior change on this feature which will take effect in next week. I will add a recording after the server side change take effect.

@norshtein norshtein requested a review from kairu-ms as a code owner November 10, 2020 09:56
@norshtein
Copy link
Member Author

Hi @arrownj , I've added recording for this change and this PR is ready to be merged. Could you please help merge the PR?

By the way, I found in the recording file, the secret of service principal is also recorded as plain text, which might need some awareness. I've deleted affected service principal, but I think some fix in recording system is needed.

@arrownj
Copy link
Contributor

arrownj commented Nov 12, 2020

Hi @arrownj Xiaojian Xu FTE , I've added recording for this change and this PR is ready to be merged. Could you please help merge the PR?

By the way, I found in the recording file, the secret of service principal is also recorded as plain text, which might need some awareness. I've deleted affected service principal, but I think some fix in recording system is needed.

Hi @norshtein , actually we can add some custom replacer while initializing a ScenarioTest. The framework has define some common used Replacer, you can read GraphClientPasswordReplacer for example. It is used in the ScenarioTest construct function. For your case, you can add a customized Replacer and add it in your ScenarioTest subclass. And if it is general enough, we can also consider add it in the test framework.

@arrownj arrownj merged commit 7ad727a into Azure:master Nov 12, 2020
@norshtein
Copy link
Member Author

@arrownj thanks for the explanation, it helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants