Skip to content

[Test Proxy] Add error response when start test proxy#26155

Merged
mccoyp merged 5 commits intoAzure:mainfrom
ponopono0322:Add-error-response-test-proxy
Nov 17, 2022
Merged

[Test Proxy] Add error response when start test proxy#26155
mccoyp merged 5 commits intoAzure:mainfrom
ponopono0322:Add-error-response-test-proxy

Conversation

@ponopono0322
Copy link
Copy Markdown
Contributor

@ponopono0322 ponopono0322 commented Sep 12, 2022

Description

Fix #26138.

If provide the -o log_cli=true option at the starting the test proxy,
example:

pytest tests/ -o log_cli=true

you can get the log:

ERROR    root:proxy_startup.py:95 Please check 'SSL_CERT_DIR' path
ERROR    root:proxy_startup.py:97 Please check 'REQUESTS_CA_BUNDLE' path

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Sep 12, 2022
@ghost
Copy link
Copy Markdown

ghost commented Sep 12, 2022

Thank you for your contribution ponopono0322! We will review the pull request and get back to you soon.

@ponopono0322 ponopono0322 changed the title [Key Vault] Add error response when start test proxy [Test Proxy] Add error response when start test proxy Sep 12, 2022
mccoyp
mccoyp reviewed Oct 14, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@mccoyp mccoyp left a comment

Choose a reason for hiding this comment

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

Thank you so much for opening this PR! I'm so sorry I didn't see this earlier, since I didn't get automatically added as a reviewer. I just have a small wording change suggestion, but this is a great change -- thanks again!

@ponopono0322
Copy link
Copy Markdown
Contributor Author

ponopono0322 commented Oct 15, 2022

By the way, this doesn't seem to be a perfect solution. Can I continue like this?

mccoyp
mccoyp approved these changes Oct 27, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@mccoyp mccoyp left a comment

Choose a reason for hiding this comment

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

Sorry about the late response again! 🙁 I think this is actually a good solution -- I considered if we should raise an actual exception instead of logging errors, but I think it's better to not block users in case they have a unique configuration. Minimizing erroneous logs by ensuring our proxy endpoint actually needs a certificate will improve this, but it's still a great change in my opinion!

@ponopono0322 @mccoyp
Update tools/azure-sdk-tools/devtools_testutils/proxy_startup.py
4ddc766 
Co-authored-by: McCoy Patiño <39780829+mccoyp@users.noreply.github.com>
@ponopono0322
Copy link
Copy Markdown
Contributor Author

ponopono0322 commented Nov 9, 2022

Thank you for your approval @mccoyp . I tried to merge it myself, but it comes out as below. May I ask for the approval of the merge?
Screenshot 2022-11-09 at 10 25 25 AM

@mccoyp
Copy link
Copy Markdown
Member

mccoyp commented Nov 17, 2022

Hi @ponopono0322, I was out of office last week but am just trying this on my end before merging to make sure it's all in order 🙂

@mccoyp mccoyp merged commit d1173e6 into Azure:main Nov 17, 2022
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Dec 1, 2022
@iscai-msft
Merge branch 'main' of https://github.com/Azure/azure-sdk-for-python …
9784d28 
…into add_multiapi_combiner

* 'main' of https://github.com/Azure/azure-sdk-for-python: (557 commits)
  [ML] Fix job.services transformation for pipeline jobs (Azure#27422)
  fix: explicitly check using is not None (Azure#27587)
  [Pipeline] Convert mldesigner primitive annotation to Input type (Azure#27539)
  code and test (Azure#27586)
  [ML][Pipelines] Enable Pipelines owned skip tests (Azure#27540)
  Update Language-Settings.ps1 (Azure#27531)
  ci: enable test_dump_distribution (Azure#27535)
  code and test (Azure#27553)
  Increment package version after release of azure-ai-textanalytics (Azure#27583)
  fix doc for autodetect default language (Azure#27577)
  [Tables] Regen with latest autorest (Azure#27205)
  [TA] regen on latest spec (Azure#27486)
  Enforce Package name approval for preview release (Azure#27573)
  Remove manual conda test stage (Azure#27576)
  [Test Proxy] Add error response when starting test proxy (Azure#26155)
  Update test_registry.py (Azure#27574)
  Explicitly disable storage blob public access for workspace templates (Azure#27396)
  Use the new audience field (Azure#27571)
  Sync eng/common directory with azure-sdk-tools for PR 4628 (Azure#27455)
  fix non-pipeline-input with annotation and default value (Azure#27492)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mccoyp mccoyp mccoyp approved these changes

Assignees

@mccoyp mccoyp

Labels

customer-reported Issues that are reported by GitHub users external to the Azure organization.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Test Proxy] Add error response with debug information when startup hangs

3 participants

@ponopono0322 @mccoyp @jeesang7