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

Update AutoRest C# version #14407

Merged
merged 30 commits into from
Aug 25, 2020
Merged

Update AutoRest C# version #14407

merged 30 commits into from
Aug 25, 2020

Conversation

azure-sdk
Copy link
Collaborator

not-specified

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

Duplicate Accept headers are being added to search.

@@ -76,6 +76,7 @@ internal HttpMessage CreateCreateOrUpdateRequest(string dataSourceName, SearchIn
request.Headers.Add("Prefer", "return=representation");
request.Headers.Add("Accept", "application/json; odata.metadata=minimal");
request.Headers.Add("Content-Type", "application/json");
request.Headers.Add("Accept", "application/json");
Copy link
Member

Choose a reason for hiding this comment

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

This still isn't fixed. We can't ship this way because it changes the output. Which, BTW, you wouldn't have to re-record - and probably shouldn't to not only reduce the git database size, but even just trying to review this PR in a browser on github.com (I'm using VSCode because the browser hangs trying to open this)

@@ -76,6 +76,7 @@ internal HttpMessage CreateCreateOrUpdateRequest(string dataSourceName, SearchIn
request.Headers.Add("Prefer", "return=representation");
request.Headers.Add("Accept", "application/json; odata.metadata=minimal");
request.Headers.Add("Content-Type", "application/json");
request.Headers.Add("Accept", "application/json");
Copy link
Member

Choose a reason for hiding this comment

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

Yes - here's the invalid output in the recordings. I'm surprised the server even responded - maybe it used the first one? We shouldn't gamble that's consistent or even supported:

  "Entries": [
    {
      "RequestUri": "https://azs-net-t50d8619e0b774b52.search.windows.net/indexes(\u0027irggcyth\u0027)/docs/search.post.autocomplete?api-version=2020-06-30",
      "RequestMethod": "POST",
      "RequestHeaders": {
        "Accept": [
          "application/json; odata.metadata=none",
          "application/json"
        ],

@pakrym
Copy link
Contributor

pakrym commented Aug 24, 2020

@heaths not planning to merge this without a bug fix. Just making sure all projects are in a re-recordable state.

@heaths
Copy link
Member

heaths commented Aug 24, 2020

@pakrym once the bug fix is in, might be good to back out the recorded sessions then and make sure the tests pass without re-recording, or is that the idea already?

@pakrym
Copy link
Contributor

pakrym commented Aug 24, 2020

@pakrym once the bug fix is in, might be good to back out the recorded sessions then and make sure the tests pass without re-recording, or is that the idea already?

Yep, that's the idea.

@@ -50,3 +56,4 @@




Copy link
Member

Choose a reason for hiding this comment

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

Nit: is the auto-updater adding a blank line to the end everytime it updates? Might be good to stop that eventually.

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

Looks like search was removed entirely, so that's good to see. Just a couple nits, but otherwise LGTM.

@pakrym pakrym merged commit 4e3bbea into Azure:master Aug 25, 2020
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.

8 participants