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.

Sign up for GitHub

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

Jump to bottom

[recorder] Zip recordings and add recordings/ to the .gitignore #5034

Closed
sadasant opened this issue Sep 6, 2019 · 5 comments
Closed

[recorder] Zip recordings and add recordings/ to the .gitignore #5034

sadasant opened this issue Sep 6, 2019 · 5 comments
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

Comments

@sadasant
Copy link
Contributor

sadasant commented Sep 6, 2019

Every time the tests are updated, new recordings need to be generated. This is extremely disruptive for PR reviews. We can avoid this by zipping the recordings and dealing with the unzip process in the background. Once we do that, we can also .gitignore the recordings folder.
@sadasant sadasant added the Client This issue points to a problem in the data-plane of the library. label Sep 6, 2019
@sadasant sadasant self-assigned this Sep 6, 2019
@HarshaNalluru
Copy link
Member

Compressing into a zip folder would mean that it might be hard to review in PRs?

@sadasant
Copy link
Contributor Author

sadasant commented Sep 6, 2019

The recordings would at this point be impossible to review in PRs, but should they be reviewed in PRs?

The value I'm trying to achieve is to reduce the thousands of lines in the diffs of PRs that are meaningless because they belong to the recordings, which will help to emphasize in the actual changes that the PR is trying to reflect.

@HarshaNalluru HarshaNalluru mentioned this issue Sep 6, 2019
Closed
62 tasks
@ramya-rao-a ramya-rao-a added the test-utils-recorder Label for the issues related to the common recorder label Jan 29, 2020
@xirzec xirzec added this to the Backlog milestone Jan 29, 2020
@ramya-rao-a
Copy link
Contributor

I believe @bterlson did something to remove the noise created by the recordings in PRs. See #6701

So, this issue can be closed?

@HarshaNalluru
Copy link
Member

Yes IMO.
Also, I came across an issue that @kurtzeborn logged this morning Azure/azure-sdk#976 to move the recordings out of the repos. Not sure if it would materialize soon.

@ramya-rao-a
Copy link
Contributor

Closing this issue after offline discussion with @sadasant

The problem of recordings causing noise in PR is solved by @bterlson's work in #6701

Azure/azure-sdk#976 will cover future work of possibly not having the recordings in the repo in the first place.

@xirzec xirzec removed this from the Backlog milestone May 18, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
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
Milestone
No milestone
Development

No branches or pull requests
4 participants
@sadasant @xirzec @HarshaNalluru @ramya-rao-a