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

Fixed Live Tests for Attestation SDK. #3366

Merged
merged 29 commits into from
Feb 24, 2022
Merged

Fixed Live Tests for Attestation SDK. #3366

merged 29 commits into from
Feb 24, 2022

Conversation

LarryOsterman
Copy link
Member

Fixed live tests.

@LarryOsterman
Copy link
Member Author

/azp run cpp - attestation

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

RickWinter
RickWinter previously approved these changes Feb 22, 2022
View reviewed changes
@LarryOsterman LarryOsterman linked an issue Feb 22, 2022 that may be closed by this pull request
WinHTTP HTTP Pipeline does not set WINHTTP_OPTION_CLIENT_CERT_CONTEXT to WINHTTP_NO_CLIENT_CERT_CONTEXT by default #3235
Closed
@LarryOsterman
Copy link
Member Author

/azp run cpp - attestation

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@LarryOsterman LarryOsterman changed the title Changed one line Fixed Live Tests for Attestation SDK. Feb 22, 2022
@LarryOsterman
Copy link
Member Author

/azp run cpp - attestation

@LarryOsterman
Copy link
Member Author

/azp run cpp - core

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@LarryOsterman
Copy link
Member Author

/azp run cpp - attestation

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@LarryOsterman
Copy link
Member Author

/azp run cpp - attestation

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@LarryOsterman
Copy link
Member Author

/azp run cpp - attestation

@LarryOsterman
Copy link
Member Author

/azp run cpp - storage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@LarryOsterman LarryOsterman dismissed stale reviews from RickWinter and Jinming-Hu February 24, 2022 20:02

Approved too early.

@benbp
Copy link
Member

benbp commented Feb 24, 2022

yml/env var changes LGTM.

@LarryOsterman
Copy link
Member Author

/azp run cpp - storage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

vhvb1989
vhvb1989 approved these changes Feb 24, 2022
View reviewed changes
Copy link
Member

@vhvb1989 vhvb1989 left a comment

Choose a reason for hiding this comment

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

Beautiful! Thank you!

@LarryOsterman LarryOsterman mentioned this pull request Feb 24, 2022
Closed
@LarryOsterman LarryOsterman enabled auto-merge (squash) February 24, 2022 22:26
@LarryOsterman LarryOsterman merged commit ddc9eb3 into Azure:main Feb 24, 2022
ahsonkhan
ahsonkhan reviewed Mar 30, 2022
View reviewed changes
sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp
WINHTTP_NO_CLIENT_CERT_CONTEXT,
0))
{
GetErrorAndThrow("Error while setting client cert context to ignore..");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Extra period

Suggested change
GetErrorAndThrow("Error while setting client cert context to ignore..");
GetErrorAndThrow("Error while setting client cert context to ignore.");

sdk/core/azure-core/inc/azure/core/internal/strings.hpp
Comment on lines +38 to +39
static std::string const ToUpper(std::string const& src) noexcept;
static unsigned char ToUpper(unsigned char const src) noexcept;
Copy link
Contributor

Choose a reason for hiding this comment

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

This API addition, as a complement to ToLower makes sense to add.

However, unlike private APIs in the _detail namespace, we cannot make breaking changes to things in _internal. Therefore, we typically only add public surface area to such headers within the SDK/Azure Core when an actual upstream service SDK package within the repo needs it. In this case, this looks to only be used by unit tests.

In general, we'd avoid adding APIs to the SDK that are only used by unit tests :)

This was referenced Apr 23, 2022
Merged
Merged
@LarryOsterman LarryOsterman deleted the larryo/fixlivetests branch June 14, 2022 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaseyCarter CaseyCarter CaseyCarter left review comments

@ahsonkhan ahsonkhan ahsonkhan left review comments

@vhvb1989 vhvb1989 vhvb1989 approved these changes

@benbp benbp benbp approved these changes

@antkmsft antkmsft antkmsft approved these changes

@Jinming-Hu Jinming-Hu Jinming-Hu left review comments

@gearama gearama Awaiting requested review from gearama gearama is a code owner

@vinjiang vinjiang Awaiting requested review from vinjiang vinjiang is a code owner

@katmsft katmsft Awaiting requested review from katmsft

@schaabs schaabs Awaiting requested review from schaabs

@RickWinter RickWinter Awaiting requested review from RickWinter RickWinter is a code owner

@danieljurek danieljurek Awaiting requested review from danieljurek danieljurek is a code owner

@weshaggard weshaggard Awaiting requested review from weshaggard weshaggard is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@LarryOsterman @benbp @CaseyCarter @RickWinter @ahsonkhan @vhvb1989 @antkmsft @Jinming-Hu