[Event Hubs] Fix environment variable names#4391
Conversation
ramya-rao-a
left a comment
ramya-rao-a
left a comment
There was a problem hiding this comment.
There is a tests.yml file that needs to be updated as well
Updated |
| const eventHubName = ""; | ||
|
|
||
| // Define AZURE_TENANT_ID, AZURE_CLIENT_ID and AZURE_CLIENT_SECRET of your AAD application in your environment | ||
| // Define EVENTHUB_AAD_TENANT_ID, EVENTHUB_AAD_CLIENT_ID and EVENTHUB_AAD_CLIENT_SECRET of your AAD application in your environment |
There was a problem hiding this comment.
Have we tested this sample after this change? I don't believe EnvironmentCredential will work as expected after this change.
The changes required from #4209 is for updating the env vars used in our tests, not the sample code or the usage of EnvironmentCredential
In this sample, we are telling our customers to use the EnvironmentCredential which depends on AZURE_TENANT_ID, AZURE_CLIENT_ID and AZURE_CLIENT_SECRET to be present. We cannot change this.
There was a problem hiding this comment.
Since this was just renaming of variables for consistency and not much change in implementation, I had run tests to verify the loading of env vars ( And not the samples or live tests (yet) )
Nothing in the code or issue surfaces that @azure/identity requires environment variables to be defined with specific names.
Looks like it's not possible to have SDK specific names as suggested at - #4209 (comment)
cc: @weshaggard
There was a problem hiding this comment.
This reverts commit f6834cd.
|
@KarishmaGhiya @danieljurek Could you merge this PR once Engineering System side of things are updated as well? |
|
/azp run js - eventhubs-client - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
LGTM, can merge once tests complete successfully |
3 participants
For more context refer to #4209
*Pending discussion on AAD related env vars