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.

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

Fixes #716 ui settings api query failing. #1782

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

real-jrw2
Copy link

@real-jrw2 real-jrw2 commented Apr 2, 2023

What this PR does / why we need it:
When using the ApplicationBuilder.UseHealthChecksUI, the ui settings middleware is never executed. The ui api endpoint middleware is currently handling all requests to "healthchecks-api/*". Moving the mapping of the ui settings middleware to before the ui api endpoint middleware mapping allows the ui settings to handle requests to "healthchecks-api/ui-settings".

This does put the ui-settings behind the request limiter, if that was not the intention then the ui settings mapping should happen before the api path is mapped.
Which issue(s) this PR fixes:
#716

Please reference the issue this PR will close: #[issue number]

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
no

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Unit tests passing
  • End-to-end tests passing
  • Extended the documentation
  • Provided sample for the feature

@github-actions github-actions bot added the UI label Apr 2, 2023
@sungam3r
Copy link
Collaborator

sungam3r commented Apr 8, 2023

@real-jrw2 Thanks for contribution. Unfortunately, now this project has little support, especially in UI-related stuff, see #1714. Also CI workflow for UI projects is broken so I tend not to merge any PRs until it will be fixed.

@sungam3r
Copy link
Collaborator

UI CI was fixed, conflicts here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants