[Recorder] `configureClientOptions` method on the recorder #20175

HarshaNalluru · 2022-02-01T22:26:48Z

Packages impacted by this PR

Anyone who's using the new recorder or the perf framework

Issues associated with this PR

#16876 #19859
Fixes #16876

Describe the problem that is addressed by this PR

Now that #19920 - "additionalPolicies array in the client options" is merged, this PR adds 725c2d9 (configureClientOptions method) for both the perf framework and the recorder.
Listing all the changes below.

Recordings should ideally not be impacted by these changes and should work fine as before.

… harshan/issue/16876

…alluru/azure-sdk-for-js into harshan/issue/16876

… harshan/issue/16876

check-enforcer · 2022-02-04T00:42:01Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

… harshan/issue/16876

HarshaNalluru · 2022-02-04T20:55:01Z

sdk/attestation/attestation/test/public/attestationTests.spec.ts

@@ -197,15 +197,15 @@ describe("[AAD] Attestation Client", function () {
    const binaryRuntimeData = base64url.decodeString(_runtimeData);
    const client = createRecordedClient(recorder, endpointType);

-    {
-      // You can't specify both runtimeData and runtimeJson.
-      await expect(


expect wasn't capturing somehow.
Maybe a breaking change or something, hence updated to assert instead.

… harshan/issue/16876

xirzec

The changes to the test contract look perfect!

Just had some minor feedback on how to clean up the issue in attestationClient.

xirzec · 2022-02-04T21:32:58Z

sdk/attestation/attestation/src/attestationClient.ts

-    } else if (clientOptions !== undefined) {
+    }
+
+    // If arg3 is defined, it has to be clientOptions and arg2 has to be a token credential


I think we can also ditch the initial assignment to options, since clientOptions has a default value that isn't undefined

I think we can simplify everything in this constructor to something like

if(credentialsOrOptions && isTokenCredential(credentialsOrOptions)) { credential = credentialsOrOptions; credentialScopes = ["https://attest.azure.net/.default"]; options = clientOptions; } else { options = credentialsOrOptions; }

perfect, updated as suggested

… harshan/issue/16876

xirzec

Changes look great! Nice that we landed on a sensible public API for this

sadasant · 2022-02-05T01:44:50Z

sdk/attestation/attestation/CHANGELOG.md

-* Renamed `AttestationResponse.value` to `AttestationResponse.body` to align with
- API guidelines.
+- Renamed `AttestationResponse.value` to `AttestationResponse.body` to align with
+  API guidelines.


Wait, are these changes necessary?

prettier did this.

sdk/test-utils/perf/CHANGELOG.md

sdk/test-utils/perf/src/batchPerfTest.ts

sdk/test-utils/recorder/CHANGELOG.md

sadasant · 2022-02-05T01:48:00Z

sdk/test-utils/recorder/CHANGELOG.md

+  Leveraging the new option, `configureClientOptions` method is added to the `Recorder`.
+  [#20175](https://github.com/Azure/azure-sdk-for-js/pull/20175)
+
+  With the support from the new `Recorder#configureClientOptions` method, we no longer need the `Recorder#configureClient` that used to access the private "pipeline" object internal to the client to add/modify the policies. [#20175](https://github.com/Azure/azure-sdk-for-js/pull/20175) removes the `Recorder#configureClient` along with the new addition.


Same here. I don’t think this indentation applies.

updated all the changelog related indentations

sadasant

Looks good! — Make sure to get another approval as well 🙂

… harshan/issue/16876

joheredi · 2022-02-07T23:43:14Z

sdk/attestation/attestation/src/attestationClient.ts

      options = clientOptions;
+    } else {
+      options = credentialsOrOptions || {};


nit: You can use ?? here. Not a big deal but it is semantically clearer.

I don't want to kick off another 100 builds again. 🥲

I'll consider the change if I am making any other significant commit 🙂

joheredi

Looks great!

timovv

Looks good!

sdk/test-utils/recorder/src/recorder.ts

jeremymeng

Looks good! One minor question

jeremymeng · 2022-02-08T00:03:33Z

sdk/test-utils/testing-recorder-new/test/noOpCredentialTest.spec.ts

+    const client = new TableServiceClient(
+      assertEnvironmentVariable("TABLES_URL"),
+      credential,
+      recorder.configureClientOptions({})


nit: is it possible to also accept undefined? I am thinking of the case where I already have an options? of the original client options type and pass it here.

Can do.
I didn't want to do that, just to force the users to pass something..
to not have any confusion when they actually want to pass options.

If you feel strongly, I can allow undefined.

Either way is fine.

HarshaNalluru added 4 commits February 1, 2022 20:45

configureClientOptions core-v2

88fe779

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

00eb82e

… harshan/issue/16876

configureClientOptions

8048e9e

& Record<string, unknown>

e1626ca

ghost added Attestation Synapse Tables Cognitive - Translator Schema Registry labels Feb 1, 2022

HarshaNalluru added 7 commits February 2, 2022 00:15

Replicate Azure#19920 for core-client-rest

a6d1b95

formatting

6c29355

Replicate Azure#19920 for core-client-rest

5885760

format

e44fe9c

Merge branch 'harshan/issue/16876-rest' of https://github.com/HarshaN…

c64883f

…alluru/azure-sdk-for-js into harshan/issue/16876

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

bf8bd52

… harshan/issue/16876

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

2eaf778

… harshan/issue/16876

HarshaNalluru changed the title ~~Harshan/issue/16876~~ [Recorder] configureClientOptions method on the recorder Feb 4, 2022

Azure deleted a comment from azure-sdk Feb 4, 2022

HarshaNalluru added 4 commits February 4, 2022 01:42

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

fe5fb23

… harshan/issue/16876

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

8822738

… harshan/issue/16876

attestation test fixes

4f84663

format

0ce0ed3

HarshaNalluru commented Feb 4, 2022

View reviewed changes

HarshaNalluru added 6 commits February 4, 2022 20:55

more format

5fc3208

format

4340a71

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

b5fa677

… harshan/issue/16876

more recorder.configureClientOptions

f65c68a

migration guide

9a9e0aa

more format

bab5665

HarshaNalluru requested a review from xirzec as a code owner February 4, 2022 21:14

xirzec reviewed Feb 4, 2022

View reviewed changes

HarshaNalluru added 2 commits February 4, 2022 23:07

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

2b36e82

… harshan/issue/16876

mixed-reality update

74684dd

HarshaNalluru requested a review from FlorianBorn71 as a code owner February 4, 2022 23:12

HarshaNalluru added 2 commits February 4, 2022 23:26

attestation simplification

c79a6bb

more fixes

23dcf48

HarshaNalluru requested a review from xirzec February 4, 2022 23:56

changelogs and gettingstarted docs

18a4fab

xirzec approved these changes Feb 5, 2022

View reviewed changes

sadasant reviewed Feb 5, 2022

View reviewed changes

sdk/test-utils/perf/CHANGELOG.md Outdated Show resolved Hide resolved

sadasant reviewed Feb 5, 2022

View reviewed changes

sdk/test-utils/perf/src/batchPerfTest.ts Outdated Show resolved Hide resolved

sadasant reviewed Feb 5, 2022

View reviewed changes

sdk/test-utils/recorder/CHANGELOG.md Outdated Show resolved Hide resolved

sadasant reviewed Feb 5, 2022

View reviewed changes

sadasant approved these changes Feb 5, 2022

View reviewed changes

HarshaNalluru added 7 commits February 7, 2022 02:11

format

3b2f286

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

8e564c1

… harshan/issue/16876

format

0215d5d

changelog

dc1a346

tables ci - test-proxy variable

1601db6

env.SAS_CONNECTION_STRING test fix

a4fe196

sort imports

d0cbb6f

HarshaNalluru requested a review from jeremymeng February 7, 2022 22:45

joheredi reviewed Feb 7, 2022

View reviewed changes

joheredi approved these changes Feb 7, 2022

View reviewed changes

timovv approved these changes Feb 7, 2022

View reviewed changes

sdk/test-utils/recorder/src/recorder.ts Show resolved Hide resolved

jeremymeng approved these changes Feb 8, 2022

View reviewed changes

HarshaNalluru merged commit c9dcb43 into Azure:main Feb 8, 2022

HarshaNalluru deleted the harshan/issue/16876 branch February 8, 2022 00:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Recorder] `configureClientOptions` method on the recorder #20175

[Recorder] `configureClientOptions` method on the recorder #20175

HarshaNalluru commented Feb 1, 2022 •

edited

Loading

check-enforcer bot commented Feb 4, 2022

HarshaNalluru Feb 4, 2022 •

edited

Loading

xirzec left a comment

xirzec Feb 4, 2022

HarshaNalluru Feb 4, 2022

xirzec left a comment

sadasant Feb 5, 2022

HarshaNalluru Feb 7, 2022 •

edited

Loading

sadasant Feb 5, 2022

HarshaNalluru Feb 7, 2022

sadasant left a comment

joheredi Feb 7, 2022

HarshaNalluru Feb 7, 2022

joheredi left a comment

timovv left a comment

jeremymeng left a comment

jeremymeng Feb 8, 2022

HarshaNalluru Feb 8, 2022

HarshaNalluru Feb 8, 2022

jeremymeng Feb 8, 2022

[Recorder] configureClientOptions method on the recorder #20175

[Recorder] configureClientOptions method on the recorder #20175

Conversation

HarshaNalluru commented Feb 1, 2022 • edited Loading

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

check-enforcer bot commented Feb 4, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

HarshaNalluru Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joheredi left a comment

Choose a reason for hiding this comment

timovv left a comment

Choose a reason for hiding this comment

jeremymeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Recorder] `configureClientOptions` method on the recorder #20175

[Recorder] `configureClientOptions` method on the recorder #20175

HarshaNalluru commented Feb 1, 2022 •

edited

Loading

HarshaNalluru Feb 4, 2022 •

edited

Loading

HarshaNalluru Feb 7, 2022 •

edited

Loading