[communication][sms] run sms samples in pipeline #15345
Conversation
ghost
added
the
|
/azp run js - communication-sms - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| const endpoint = | ||
| process.env["COMMUNICATION_ENDPOINT"] || "https://<resource-name>.communication.azure.com"; | ||
| process.env.COMMUNICATION_ENDPOINT || |
There was a problem hiding this comment.
Do we want to just remove this process.env.COMMUNICATION_ENDPOINT? We don't even have a reference to it in the sample.env.
There was a problem hiding this comment.
I'll add COMMUNICATION_ENDPOINT to sample.env because if you're using aad to authenticate you most likely won't have a connection string to parse. Also, if a developer has a connection string they might as well use it to authenticate instead of aad. We only have connection string in this sample because we don't have COMMUNICATION_ENDPOINT.
| @@ -51,7 +51,7 @@ node sendSms.js | |||
| Alternatively, run a single sample with the correct environment variables set (setting up the `.env` file is not required if you do this), for example (cross-platform): | |||
|
|
|||
| ```bash | |||
| npx cross-env COMMUNICATION_CONNECTION_STRING="<communication connection string>" node sendSms.js | |||
| npx cross-env COMMUNICATION_SAMPLES_CONNECTION_STRING="<communication samples connection string>" FROM_PHONE_NUMBER="<from phone number>" AZURE_PHONE_NUMBER="<azure phone number>" TO_PHONE_NUMBERS="<to phone numbers>" AZURE_PHONE_NUMBER="<azure phone number>" node sendSms.js | |||
There was a problem hiding this comment.
AZURE_PHONE_NUMBER="<azure phone number>" node sendSms.js
I think these instructions are a little unclear. Cause technically if we have TO_PHONE_NUMBERS and FROM_PHONE_NUMBERS then setting the AZURE_PHONE_NUMBER become moot. We should strive for a bit more clarity.
There was a problem hiding this comment.
this is auto generated
There was a problem hiding this comment.
only way to be clearer is to not use AZURE_PHONE_NUMBER
There was a problem hiding this comment.
I could edit the generated output but then this would have to be done every time the publish command is run
There was a problem hiding this comment.
We want to keep AZURE_PHONE_NUMBER cause that is the one at the moment that the pipeline is injecting. I can make a new property in the config for the new properties TO_PHONE_NUMBERS and FROM_PHONE_NUMBERS but for the time being this should be fine
|
|
||
| # The endpoint to the Communication Services resource | ||
| COMMUNICATION_ENDPOINT="https://<resource name>.communication.azure.net/" |
There was a problem hiding this comment.
is this being set in the LiveTest pipeline? should we make the name more in-line with the others? e,g, COMMUNICATION_SAMPLES_ENDPOINT?
|
|
||
| // construct send request | ||
| const sendRequest: SmsSendRequest = { | ||
| from: process.env.FROM_PHONE_NUMBER || process.env.AZURE_PHONE_NUMBER || "<from-phone-number>", |
There was a problem hiding this comment.
is AZURE_PHONE_NUMBER the one used in the livetest pipeline? maybe we should figure out a better way than all these or statements because I think this will confuse the customers
There was a problem hiding this comment.
@JoshuaLai @0rland0Wats0n how much effort would it be to make a subconfig for samples env vars
so maybe something like:
COMMUNICATION_SAMPLES_CONNECTION_STRING
COMMUNICATION_SAMPLES_ENDPOINT
SAMPLES_PHONE_NUMBER (alternatively we can keep this as AZURE_PHONE_NUMBER and update the sample.env)
SAMPLES_DESTINATION_PHONE_NUMBERS (change the TO_PHONE_NUMBERS in sample.env)
| const credential: TokenCredential = isNode | ||
| ? new DefaultAzureCredential() | ||
| : new ClientSecretCredential( | ||
| process.env.AZURE_TENANT_ID, |
There was a problem hiding this comment.
we would need to add this to the samples.env then, and add a small comment on what they're used for (e.g. used for AAD samples)
This PR refactors the sms samples and stops skipping them in pipeline runs.