Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Recorder] Remove function bunch - setReplaceableVariables, setReplacements, skipQueryParams #7133

Conversation

HarshaNalluru
Copy link
Member

@HarshaNalluru HarshaNalluru commented Jan 28, 2020

Changes in the PR

Changes in the recorder package

  • Removing the variables replaceableVariables, queryParameters, replacements that are being globally set.
  • Instead, scoping them to the BaseRecorder class and passing them as arguments to the functions that use them whenever required.
  • And thus no need for the function bunch - setReplaceableVariables, setReplacements, skipQueryParams and hence removed the three functions.
  • Introduced a new setEnvironmentVariables, which is as of now used for playback only (there are plans for its usage in soft-record).
  • Added tests for setEnvironmentVariables.
  • Updated the tests and some minor changes here and there accordingly.

Changes in recorder, storage and keyvault packages

  • Rename replaceInRecordings -> customizationsOnRecordings
    • Renamed replaceInRecordings attribute of RecorderEnvironmentSetup to customizationsOnRecordings which I think is more appropriate.
      (And also I couldn't wrap my head around too many replace- prefixed words.)
    • Updated changelog for this rename.
    • Updated all the seven references of replaceInRecordings in the storage and keyvault tests.

@HarshaNalluru HarshaNalluru marked this pull request as ready for review January 29, 2020 00:43
@HarshaNalluru HarshaNalluru changed the title [WIP] [Draft] [Recorder] Remove function bunch - setReplaceableVariables, setReplacements, skipQueryParams [Recorder] Remove function bunch - setReplaceableVariables, setReplacements, skipQueryParams Jan 29, 2020
@HarshaNalluru HarshaNalluru added Client This issue points to a problem in the data-plane of the library. test-utils-recorder Label for the issues related to the common recorder labels Jan 29, 2020
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

:shipit:

Add [BREAKING] prefix to the statement.
env.SECRET = "SECRET";
const replaceableVariables = {};

setEnvironmentVariables(replaceableVariables);
Copy link
Contributor

Choose a reason for hiding this comment

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

Once setEnvironmentVariables has the environment object parametrized, this unit test won't need to change the global environment object!

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, but still changing the global environment object to actually test the feature.

sdk/test-utils/recorder/test/utils.spec.ts Show resolved Hide resolved
sdk/test-utils/recorder/test/utils.spec.ts Show resolved Hide resolved
@@ -14,7 +14,7 @@ export async function authenticate(that: any): Promise<any> {
AZURE_TENANT_ID: "azure_tenant_id",
KEYVAULT_NAME: "keyvault_name"
},
replaceInRecordings: [
customizationsOnRecordings: [
(recording: any): any =>
Copy link
Member Author

Choose a reason for hiding this comment

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

Heads up on the rename, @willmtemple !!

Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

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

Looks good!

…to harshan/PR-followup/7083-stg-q-remove-funcs
@HarshaNalluru HarshaNalluru merged commit bbdf112 into Azure:master Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. test-utils-recorder Label for the issues related to the common recorder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants