Skip to content

[Test] Traffic is not recorded by default during RGPreparer setup.#9431

Merged
adewaleo merged 15 commits intoAzure:devfrom
adewaleo:resource-group-preparer-no-record
Jun 6, 2019
Merged

[Test] Traffic is not recorded by default during RGPreparer setup.#9431
adewaleo merged 15 commits intoAzure:devfrom
adewaleo:resource-group-preparer-no-record

Conversation

@adewaleo
Copy link
Contributor

@adewaleo adewaleo commented May 17, 2019

Resource Group Preparer is no longer recorded. Moreover group create and group delete logic will be ignored if the test is not run live.

This can be extended to other preparers as well. It might be a bit more complicated if the preparer returns information that isn't an input. E.g. account_key in the case of the StorageAccountPreparer. But it is doable. In the case of the StorageAccountPreparer, we can solve this problem by returning the recording replacement of the account key.

closes #9272


This checklist is used to make sure that common guidelines for a pull request are followed.

  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

  • I adhere to the Command Guidelines.

Copy link
Member

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

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

I feel this solution should be more generic since NONE of the preparers should record traffic, not just resource group. However, if the cost-benefit analysis does justify the extra work, this should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

The intent with preparers is that none of the traffic for their creation is recorded. They are "assumed to exist" on playback. Thus, this fix should really be implemented in some kind of base class, because it should equally apply to StorageAccountPreparers and all the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, this makes sense. The other preparers might be complicated as they sometimes rely on nondeterministic info (at least StorageAccountPreparers), but it is definitely doable.

I will make the changes.

@adewaleo adewaleo requested a review from marstr May 21, 2019 06:36
@adewaleo adewaleo force-pushed the resource-group-preparer-no-record branch from 8aa2209 to d2a962c Compare May 25, 2019 03:30
@adewaleo adewaleo requested a review from limingu May 25, 2019 23:46
@adewaleo
Copy link
Contributor Author

adewaleo commented May 28, 2019

I think this should be good to go @tjprescott @yugangw-msft

Is there any need for updated documentation?

Also, it would be best to merge this after the release.

@adewaleo adewaleo force-pushed the resource-group-preparer-no-record branch from c1a8700 to 72b57cf Compare June 3, 2019 17:17
@adewaleo adewaleo force-pushed the resource-group-preparer-no-record branch from 2bcd303 to cde9c82 Compare June 5, 2019 03:27
Oluwatosin Adewale added 2 commits June 5, 2019 13:13
@adewaleo adewaleo merged commit 3f349c4 into Azure:dev Jun 6, 2019
adewaleo pushed a commit to adewaleo/azure-cli that referenced this pull request Jun 7, 2019
@adewaleo adewaleo mentioned this pull request Jun 7, 2019
2 tasks
adewaleo added a commit that referenced this pull request Jun 7, 2019
* undo #9431 test preparer update.

* Fix failing recordings/tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants