Skip to content

Speed up proxy selection code#599

Merged
openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
smarthall:improve_proxy_experience
Feb 3, 2025
Merged

Speed up proxy selection code#599
openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
smarthall:improve_proxy_experience

Conversation

@smarthall
Copy link
Copy Markdown
Member

@smarthall smarthall commented Jan 22, 2025

What type of PR is this?

feature

What this PR does / Why we need it?

This change speeds up the proxy selection code by performing tests on multiple proxies simultaneously.

Which Jira/Github issue(s) does this PR fix?

Resolves https://issues.redhat.com/browse/OSD-22383

Special notes for your reviewer

Unit Test Coverage

Guidelines

  • If it's a new sub-command or new function to an existing sub-command, please cover at least 50% of the code
  • If it's a bug fix for an existing sub-command, please cover 70% of the code

Test coverage checks

  • Added unit tests
  • Created jira card to add unit test
  • This PR may not need unit tests

Pre-checks (if applicable)

  • Ran unit tests locally
  • Validated the changes in a cluster
  • Included documentation changes with PR

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2025
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jan 22, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@smarthall
Copy link
Copy Markdown
Member Author

/test all

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 23, 2025

Codecov Report

❌ Patch coverage is 71.69811% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.98%. Comparing base (3e3ab00) to head (8506ceb).
⚠️ Report is 421 commits behind head on main.

Files with missing lines Patch % Lines
pkg/cli/config/config.go 71.69% 14 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #599      +/-   ##
==========================================
+ Coverage   45.81%   46.98%   +1.16%     
==========================================
  Files          86       86              
  Lines        6742     6964     +222     
==========================================
+ Hits         3089     3272     +183     
- Misses       3274     3289      +15     
- Partials      379      403      +24     
Files with missing lines Coverage Δ
pkg/cli/config/config.go 78.18% <71.69%> (-4.90%) ⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@smarthall
Copy link
Copy Markdown
Member Author

/retest

@smarthall smarthall marked this pull request as ready for review January 30, 2025 23:09
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 30, 2025
@smarthall smarthall force-pushed the improve_proxy_experience branch from 650f27b to 6c89187 Compare January 30, 2025 23:13
* Test proxies in parallel
* If there is only a single proxy congiured, return immediately
* Only take as long as it takes for the fastest proxy to respond
* Log proxy test results for easy debugging
@smarthall smarthall force-pushed the improve_proxy_experience branch from 6c89187 to 8506ceb Compare January 30, 2025 23:13
@xiaoyu74
Copy link
Copy Markdown
Contributor

Great work Daniel! The logic is clear and well meet the done criteria cases.

Tested against a prod cluster to update my proxy configuration:

cat ~/.config/backplane/config.json
{
    "proxy-url": "",
    "vpn-check-endpoint": "https://gitlab.cee.redhat.com/service/backplane-api",
    "proxy-check-endpoint": "https://api.backplane.openshift.com/healthz"
}

 ./ocm-backplane login cf9aa592-84fc-4e1c-a62e-3d48e4d4f5bd -v
WARN[0000] Your Backplane CLI is not up to date. Please run the command 'ocm backplane upgrade' to upgrade to the latest version  Current version="(devel)" Latest version=0.1.38
INFO[0001] Backplane URL retrieved via OCM environment: https://api.backplane.openshift.com
INFO[0001] No PagerDuty API Key configuration available. This will result in failure of `ocm-backplane login --pd <incident-id>` command.
INFO[0004] Target cluster                                ID=2feuradf8odo2uqdtn8t9uvr5278hchs Name=kflux-prd-rh02
INFO[0009] Backplane URL retrieved via OCM environment: https://api.backplane.openshift.com
INFO[0009] No PagerDuty API Key configuration available. This will result in failure of `ocm-backplane login --pd <incident-id>` command.
ERRO[0045] cannot connect to Backplane API URL: Head "https://api.backplane.openshift.com": context deadline exceeded (Client.Timeout exceeded while awaiting headers).

NOTE: To troubleshoot the connectivity issues, please run `ocm-backplane health-check`

Just some edge cases not covered (to mock httptest.NewServer for the testProxy) but shouldn't be a blocker to merge.
image

I will give LGTM but just hold for extra eyes from other colleague

/lgtm
/hold

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 31, 2025
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jan 31, 2025
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jan 31, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarthall, xiaoyu74

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2025
@samanthajayasinghe
Copy link
Copy Markdown
Contributor

Great refactoring @smarthall ,
@xiaoyu74 seems the test covers 69.81% of new code, I feel it's good enough for now

@xiaoyu74
Copy link
Copy Markdown
Contributor

xiaoyu74 commented Feb 3, 2025

/hold cancel

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2025
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Feb 3, 2025

@smarthall: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot Bot merged commit 1803100 into openshift:main Feb 3, 2025
@smarthall smarthall deleted the improve_proxy_experience branch February 3, 2025 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants