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

♻️ Traefik rules: final Path needed for viewing files #2737

Merged

Conversation

sanderegg
Copy link
Member

What do these changes do?

  • The final missing rule that was missing so that tests for /view entrypoint is again functional

Related issue/s

How to test

Checklist

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #2737 (112aee6) into master (fe7b308) will decrease coverage by 0.9%.
The diff coverage is n/a.

❗ Current head 112aee6 differs from pull request most recent head 6eb5b0c. Consider uploading reports for the commit 6eb5b0c to get more accurate results
Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2737     +/-   ##
========================================
- Coverage    78.2%   77.3%   -1.0%     
========================================
  Files         647     647             
  Lines       26915   26915             
  Branches     2617    2617             
========================================
- Hits        21066   20819    -247     
- Misses       5143    5367    +224     
- Partials      706     729     +23     
Flag Coverage Δ
integrationtests 65.6% <ø> (+<0.1%) ⬆️
unittests 73.4% <ø> (-0.6%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ce_director_v2/modules/db/repositories/clusters.py 48.1% <0.0%> (-37.1%) ⬇️
...simcore_service_director_v2/api/routes/clusters.py 58.8% <0.0%> (-35.3%) ⬇️
...e_service_director_v2/api/dependencies/__init__.py 66.6% <0.0%> (-33.4%) ⬇️
...mcore_service_director_v2/api/dependencies/dask.py 75.0% <0.0%> (-25.0%) ⬇️
...s/dynamic_sidecar/docker_service_specs/settings.py 48.7% <0.0%> (-23.8%) ⬇️
...service_director_v2/api/routes/dynamic_services.py 65.8% <0.0%> (-21.6%) ⬇️
...simcore_service_director_v2/api/routes/services.py 82.3% <0.0%> (-17.7%) ⬇️
...e_director_v2/api/dependencies/dynamic_services.py 82.6% <0.0%> (-17.4%) ⬇️
...simcore_service_director_v2/modules/dask_client.py 66.8% <0.0%> (-14.7%) ⬇️
..._service_director_v2/api/dependencies/scheduler.py 85.7% <0.0%> (-14.3%) ⬇️
... and 21 more

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

Would it make sense to add a test in osparc-simcore/tests/swarm-deploy to check these path are in sync?

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

Let me look at it in more dtail and provide you feedback in amoment

@@ -229,7 +229,7 @@ services:
- traefik.http.routers.${SWARM_STACK_NAME}_webserver.rule=hostregexp(`{host:.+}`)
&& (Path(`/`, `/v0`,`/socket.io/`,`/static-frontend-data.json`,
`/study/{study_uuid:\b[0-9a-f]{8}\b-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-\b[0-9a-f]{12}\b}`,
`/view`) || PathPrefix(`/v0/`))
`/view`, `/#/view`) || PathPrefix(`/v0/`))
Copy link
Member

@pcrespov pcrespov Jan 13, 2022

Choose a reason for hiding this comment

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

@sanderegg I am afraid is not the last ... viewers (studies_dispatcher plugin) as a redirection syntax that uses fragments to communicate with the front end.
For instance

https://osparc.io/#/error?message=Sorry%2C%20I%20could%20not%20find%20this%20&status_code=404

Copy link
Member

@pcrespov pcrespov Jan 13, 2022

Choose a reason for hiding this comment

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

This builds the redirect response and adds a fragment to a page with a query (similar to portainer) . There are two pages, namely view and error so far, so I guess the label pattern is something like

Path(`/view`, `/error`) || PathPrefix(`/#`)

the two first will return different front-ends (/ -> osparc, /view, /error -> views) and fragments is info that the front end will consume

Copy link
Member

@pcrespov pcrespov Jan 13, 2022

Choose a reason for hiding this comment

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

Actually we use fragments for much more at the root as well

https://osparc.io/#reset-password?code=131234
https://osparc.io/#reset-password?registered
https://osparc.io/#/registration/?invitation={code}

at the time, i already suggesting standardizing this #1975
Perhaps it is the moment to do so.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, so for the /view and /error I will set on top /view/#/ and /error/#/. for the last 3 ones I'm puzzled.

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

thx for looking at this L:-)

@sanderegg sanderegg merged commit 1f76deb into ITISFoundation:master Jan 13, 2022
@sanderegg sanderegg deleted the e2e/add_final_path_for_viewer branch January 13, 2022 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants