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] Followup of #7083 - Coupling RecorderEnvironmentSetup with the record call #7085

Merged

Conversation

HarshaNalluru
Copy link
Member

@HarshaNalluru HarshaNalluru commented Jan 24, 2020

The changes in this PR are as per the discussions with @xirzec and @sadasant .

Changes in the PR

@sadasant
Copy link
Contributor

Are we adding CHANGELOG entries to the recorder? I wonder if we should start.

Other than that, once this PR works, I think we'll be good to go. This is probably the happiest path at the moment.

Also, any thoughts on unit tests for this?

@HarshaNalluru
Copy link
Member Author

HarshaNalluru commented Jan 24, 2020

Are we adding CHANGELOG entries to the recorder? I wonder if we should start.

Good suggestion. I was thinking too.
We are not versioning the package since this is not published.

Still, a changelog with just the dates would be good for keeping track things.

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.

Usage looks good too!

@HarshaNalluru HarshaNalluru marked this pull request as ready for review January 28, 2020 01:05
@HarshaNalluru HarshaNalluru added Client This issue points to a problem in the data-plane of the library. KeyVault Storage Storage Service (Queues, Blobs, Files) labels Jan 28, 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:

],
queryParametersToSkip: []
};
const recorder = record(that, recorderEnvSetup);
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this!!!!

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! Thank you!

@HarshaNalluru
Copy link
Member Author

Tested the playback locally for all the storage and keyvault packages at 0fb53c7.

Copy link
Member

@jeremymeng jeremymeng 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!

@HarshaNalluru HarshaNalluru merged commit 0ce15cc into Azure:master Jan 28, 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. KeyVault Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants