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

Honor metadata etag for get_certs path. #1401

Merged
merged 4 commits into from
Dec 4, 2018
Merged

Conversation

adepue
Copy link
Member

@adepue adepue commented Nov 20, 2018

Description

Issue #


PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes in the description.
  • New Unit tests were added for the changes made and Travis.CI is passing.

Quality of Code and Contribution Guidelines

@adepue
Copy link
Member Author

adepue commented Nov 20, 2018

@narrieta for feedback

@narrieta
Copy link
Member

Change looks ok. Running automation.

@adepue
Copy link
Member Author

adepue commented Nov 21, 2018

@narrieta Seems that the etag value wasn't ever being passed up to the server either. I have added another commit here that addresses that. Let me know what you think.

@adepue
Copy link
Member Author

adepue commented Nov 21, 2018

The entire restutil class here doesnt seem to honor the 304 response codes per the protocol. I am going to close this PR for now and will bring it back once we have fully verified the failure patterns. Sorry for the scramble @narrieta

@adepue adepue closed this Nov 21, 2018
@narrieta
Copy link
Member

@adepue No problem, thanks!

@adepue adepue reopened this Nov 21, 2018
@adepue
Copy link
Member Author

adepue commented Nov 21, 2018

@narrieta I have updated to incorporate handling of 304 return codes in a way isolated to the metadata protocol only. Let me know what you think of the code structure. We have a setup we can test on once we are fine with the code approach.

@adepue
Copy link
Member Author

adepue commented Nov 27, 2018

@narrieta Let me know if you have concerns with this approach/update at all.

@narrieta
Copy link
Member

Looks good. Running automation

@narrieta narrieta merged commit 9e34933 into Azure:master Dec 4, 2018
@vrdmr vrdmr added this to the v2.2.35 milestone Dec 22, 2018
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.

3 participants