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

Implementation for GetKeys, GetVersions, GetDeletedKeys #6479

Merged
merged 3 commits into from
Jun 7, 2019
Merged

Implementation for GetKeys, GetVersions, GetDeletedKeys #6479

merged 3 commits into from
Jun 7, 2019

Conversation

maririos
Copy link
Member

@maririos maririos commented Jun 4, 2019 •
edited
Loading

It depends on PR #6457 so while that one gets merged, marking this one as draft.

Actual changes for this PR are in commit e6db481

Update: PR has been updated to include latest changes in the Keys API.
It is ready to review

@maririos maririos requested review from AlexGhiondea and schaabs June 4, 2019 23:03
@maririos maririos mentioned this pull request Jun 5, 2019
Merged
@maririos maririos marked this pull request as ready for review June 6, 2019 21:50
@maririos
Copy link
Member Author

maririos commented Jun 6, 2019

Update: PR has been updated to include latest changes in the Keys API.
It is ready to review

@maririos
Copy link
Member Author

maririos commented Jun 6, 2019

cc: @AlexGhiondea @schaabs

@maririos maririos requested review from pakrym and jsquire June 7, 2019 14:57
@maririos
Copy link
Member Author

maririos commented Jun 7, 2019

All feedback has been addressed

@jsquire
Copy link
Member

jsquire commented Jun 7, 2019

All feedback has been addressed

@maririos
Copy link
Member Author

maririos commented Jun 7, 2019

CI failure not related to my changes. I'm already talking to Eng Services about it.
If there isn't more feedback, could I get someone to approve it? :)

@mitchdenny
Copy link
Contributor

Hey folks - looking into this now.

@mitchdenny
Copy link
Contributor

mitchdenny commented Jun 7, 2019 •
edited
Loading

So the first part of the mystery around why each pipeline is running:

Pipeline Why it ran
azure-sdk-for-net - mgmt It always runs :(
net - core - ci Because you changed files in sdk/core/
net - cognitiveservices - ci Because where were changes in master here since you took your branch.
net - keyvault - ci Because you changed files in sdk/keyvault/
net - batch - ci Because where were changes in master here since you took your branch.
net - client - ci Triggered for same reason as above, but had a config error (fixed)

I'm going to take a closer look at some of the failures and re-run a few pipelines just in case there is some layered problems here.

@mitchdenny
Copy link
Contributor

/azp run net - client - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

pakrym
pakrym reviewed Jun 7, 2019
View reviewed changes
sdk/keyvault/Azure.Security.KeyVault.Keys/src/KeyClient_private.cs
return request;
}

private Uri CreateFirstPageUri(string path)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't really help, to be efficient AppendQuery needs to be called on request.UriBuilder but it might require way too much refactoring for this PR.

@maririos maririos merged commit 756b072 into Azure:master Jun 7, 2019
@maririos maririos deleted the lists branch July 11, 2019 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsquire jsquire jsquire left review comments

@pakrym pakrym pakrym approved these changes

@AlexGhiondea AlexGhiondea Awaiting requested review from AlexGhiondea

@schaabs schaabs Awaiting requested review from schaabs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maririos @jsquire @mitchdenny @pakrym