Skip to content

Support non-default ports for the Discovery Service #116

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
Merged
merged 9 commits into from
Jul 18, 2025
Merged

Support non-default ports for the Discovery Service #116

merged 9 commits into from
Jul 18, 2025

Conversation

@jfriedri-ni
Copy link
Collaborator

@jfriedri-ni jfriedri-ni commented Jul 16, 2025 •
edited
Loading

What does this Pull Request accomplish?

  • Use the discovery client to resolve the discovery service's URL
  • Use the resolved URL when initializing the refresh component
  • Perform the resolution during initialize_refresh_component()

Why should this Pull Request be merged?

This is the client-side update for AB#3191854

Without this update, the user sees this message in the browser when their discovery service is not on the default port:

Your app is having trouble loading the nipanel.streamlit_refresh.panelRefreshComponent component. The app is attempting to load the component from http://localhost:{PORT}/panel-service/refresh/performance_checker_panel, and hasn't received its Streamlit.setComponentReady() message.

What testing has been done?

  • PR checks
  • Edited the discovery service's settings json file and changed the port number to 42801
    • Ran the performance checker and inspected the sources
    • The refresh component uses the updated port number
image

@jfriedri-ni
Resolve the discovery service and use it as the base URL for the refr…
f21d54a 
…esh component

- Use the resolved URL when initializing the refresh component
- Perform the resolution in PanelClient and expose it as a property

Signed-off-by: Joe Friedrichsen <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Jul 16, 2025 •
edited
Loading

Test Results

   10 files  ±0     10 suites  ±0   21s ⏱️ -10s
  221 tests ±0    221 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 160 runs  ±0  2 160 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit acb1a58. ± Comparison against base commit 49c97c1.

♻️ This comment has been updated with latest results.

mikeprosserni
mikeprosserni approved these changes Jul 17, 2025
View reviewed changes
bkeryan
bkeryan requested changes Jul 17, 2025
View reviewed changes
csjall
csjall requested changes Jul 17, 2025
View reviewed changes
@jfriedri-ni jfriedri-ni requested review from bkeryan and csjall July 18, 2025 18:25
bkeryan
bkeryan requested changes Jul 18, 2025
View reviewed changes
@jfriedri-ni jfriedri-ni requested a review from bkeryan July 18, 2025 20:09
bkeryan
bkeryan approved these changes Jul 18, 2025
View reviewed changes
Copy link
Collaborator

@bkeryan bkeryan left a comment

Choose a reason for hiding this comment

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

Approved with suggestions

@jfriedri-ni jfriedri-ni merged commit b1c1605 into main Jul 18, 2025
14 checks passed
@jfriedri-ni jfriedri-ni deleted the users/jfriedri/ab3191854-resolve-discovery-service-revproxy-port branch July 18, 2025 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikeprosserni mikeprosserni mikeprosserni approved these changes

@csjall csjall csjall approved these changes

@bkeryan bkeryan bkeryan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jfriedri-ni @csjall @bkeryan @mikeprosserni