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

Create initial test-resources.json for tables #11538

Merged
merged 5 commits into from
Apr 24, 2020

Conversation

christothes
Copy link
Member

No description provided.

@christothes christothes requested a review from jsquire April 23, 2020 15:20
@christothes christothes self-assigned this Apr 23, 2020
@christothes christothes requested a review from pakrym April 23, 2020 15:26
@pakrym
Copy link
Contributor

pakrym commented Apr 23, 2020

This looks good but I would merge it along with at least one test that uses it.

Do you have live pipeline setup?

Copy link
Member

@jsquire jsquire left a comment

Choose a reason for hiding this comment

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

LGTM

@christothes
Copy link
Member Author

Do you have live pipeline setup?

I only have the auto generated ones for CI etc. I was planning to work on setting up the pipeline next.

{
request.Headers.Add("x-ms-client-request-id", requestId);
}
// Delete requests fail without this header.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add it to swagger?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's debatable. I think the answer is yes.

Comparing notes with what happens in python, this header appears to not be required, although is harmless if added. In addition, I was able to run fiddler against Azure Storage Explorer in the portal, and it sends this header. The documentation doesn't mention it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's comment on the swagger PR at least and see how that goes.

@christothes christothes merged commit 34dc4b3 into Azure:master Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants