enforce valid custom locations SP app object id#6677
Conversation
️✔️Azure CLI Extensions Breaking Change Test
|
microsoft-github-policy-service
bot
requested review from
jsntcy,
yanzhudd,
yonzhan and
zhoxing-ms
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
added
Connected Kubernetes
Auto-Assign
TheOnlyWei
force-pushed
the
require-oid-for-cl-feature
branch
from
c056352 to
20f923d
Compare
TheOnlyWei
changed the title
TheOnlyWei
force-pushed
the
require-oid-for-cl-feature
branch
3 times, most recently
from
7401894 to
cad3189
Compare
TheOnlyWei
force-pushed
the
require-oid-for-cl-feature
branch
2 times, most recently
from
9ebfb6d to
b315d43
Compare
TheOnlyWei
force-pushed
the
require-oid-for-cl-feature
branch
3 times, most recently
from
098ac34 to
7d0e5bb
Compare
| with open(config_path, 'r') as f: | ||
| CONFIG = json.load(f) |
There was a problem hiding this comment.
File "/mnt/vss/_work/1/s/src/connectedk8s/azext_connectedk8s/tests/latest/test_connectedk8s_scenario.py", line 29, in
with open(configPath, 'r') as f:
FileNotFoundError: [Errno 2] No such file or directory: '/mnt/vss/_work/1/s/src/connectedk8s/azext_connectedk8s/tests/latest/config.json'
connectedk8s/azext_connectedk8s/tests/latest/config.json file was used in the testing of CI pipeline, why do you need to remove this file? Could you keep a config file which does not contain sensitive information to make the CI tests pass?
There was a problem hiding this comment.
No this is not possible because this is a live test. You need passwords. Luckily, the code doesn't verify that the password actually works, so I can use a fake password for now just for testing. This might not be the case in the long-run.
Ideally, Azure CLI CI pipeline should have a way to configure secrets for live tests, but I don't know if Azure CLI pipeline actually runs these tests, or ignore missing configurations files related to manual user configurations.
There was a problem hiding this comment.
Luckily, the code doesn't verify that the password actually works, so I can use a fake password for now just for testing.
OK, this is exactly the way I want to recommend
There was a problem hiding this comment.
Ideally, Azure CLI CI pipeline should have a way to configure secrets for live tests, but I don't know if Azure CLI pipeline actually runs these tests, or ignore missing configurations files related to manual user configurations.
The CI pipeline will only run tests in playback mode instead of live mode. Regarding the question of whether the CI pipeline can support configuring secrets files, add our CI expert @wangzelin007 to help take a look
There was a problem hiding this comment.
We cannot store secrets for security reasons.
If you want run live test successfully, you should create secrets in real time, then complete your test and delete all resources immediately after the test is completed.
This way secrets only exist in memory and will no longer exist after the test is completed, which is the safest.
There was a problem hiding this comment.
@wangzelin007 This is caused by new manual config file added to the live test to begin with. Azure CLI PR pipeline runs and doesn't see config and throws an error because a user needs to create the file. Ideally either there should be an option to ignore these config files or for Azure CLI PR pipeline to support secret configurations. Configurations and code should always be separate, and such configurations should obviously not be committed.
The work-around for now is to put in fake values because the code doesn't validate the password, but that is not a long-term solution if you have to write live tests where credentials are validated.
Does the Azure CLI pipeline only run recorded tests? It tried to execute new test code from this PR, so I assume not everything is recorded. As I did not re-record the test since I was only told to run the live test by owners of this extension, which I am contributing to.
There was a problem hiding this comment.
CLI PR will only run recording tests, but we have a pipeline that runs live tests once a week, and you can view the latest test results here.
For your extension, they will only run live tests because of the @live_only decorator.
For live test that requires secret configurations, we recommend that you use az keyvault and az storage commands to create these secrets in real time and destroy them after the test is completed.
There was a problem hiding this comment.
@wangzelin007 The issue with using Azure Key Vault is now you need credentials for Azure Key Vault. Since we don't need real password for the tests currently, temporary real password feature can be implemented for another PR when the need arises.
@zhoxing-ms Are there any other concerns?
There was a problem hiding this comment.
No, I have approved this PR
|
@keystroke Could you please help review this PR agian? |
|
Please resolve the code conflicts. |
TheOnlyWei
force-pushed
the
require-oid-for-cl-feature
branch
from
7d0e5bb to
48b449a
Compare
add .gitignore and test configuration file
TheOnlyWei
force-pushed
the
require-oid-for-cl-feature
branch
from
48b449a to
546ae67
Compare
|
Hi @zhoxing-ms can this be merged? |
|
[Release] Update index.json for extension [ connectedk8s ] : https://dev.azure.com/azclitools/internal/_build/results?buildId=93455&view=results |
7 participants
enforce valid custom locations SP app object id
add .gitignore and test configuration file
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
az connectedk8s
General Guidelines
Style errors from the command were already there. Didn't seem style guide was previously enforced, so won't fix these errors here.
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally?