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

feat: add Docker image + sample_project entrypoint & setup #877

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

b1rger
Copy link
Contributor

@b1rger b1rger commented May 29, 2024

Closes: #602

@b1rger b1rger force-pushed the birger/602-dockerimage branch from 4446c27 to be52dd3 Compare May 29, 2024 07:17
@b1rger b1rger marked this pull request as ready for review May 29, 2024 07:18
@b1rger b1rger force-pushed the birger/602-dockerimage branch from be52dd3 to eacb6ea Compare May 29, 2024 07:27
@b1rger b1rger requested a review from sennierer May 29, 2024 07:29
Copy link
Collaborator

@sennierer sennierer left a comment

Choose a reason for hiding this comment

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

with apis_core.urls added to /apis this is not working out of the box. In the sample_project we should include apis_core.urls as the root: urlpatterns = [path("", include("apis_core.urls", namespace="apis"))] instead.
Also with the current setup creation of any links in the sample_project fails due to #884
So we should either rename the sample_project (at least in the docker-image) or fix #884 before merging this.

@b1rger
Copy link
Contributor Author

b1rger commented Jun 4, 2024

with apis_core.urls added to /apis this is not working out of the box. In the sample_project we should include apis_core.urls as the root: urlpatterns = [path("", include("apis_core.urls", namespace="apis"))] instead.

I think we should fix #876 instead. Including the apis URIs in a specific namespace should not be a requirement for a running instance.

@sennierer
Copy link
Collaborator

makes sense. But my comment was regarding the path where apis_core.urls is currently included in the sample_project. Given that it is included under /apis there is no url resolving under / and the login is not correctly redirecting. So no matter if we get rid of the apis namespace, the path of the url include should be the root instead of /apis.

@b1rger
Copy link
Contributor Author

b1rger commented Jun 4, 2024

But my comment was regarding the path where apis_core.urls is currently included in the sample_project. Given that it is included under /apis there is no url resolving under / and the login is not correctly redirecting.

Ah, oke, thanks for clarifying, then we are on the same page, that it should work with whereever we include apis_core.urls under (be it "", "/apis", "/foobar" or whatever).

I can not reproduce the login not redirecting correctly - how did you login?

(I can reproduce the logout not redirecting correctly, though - this seems to be this problem, although I'm not sure why that did not come up yet)

@sennierer
Copy link
Collaborator

when the url is at "/apis", you are not logged in and you click on lets say "Person" (to go to the list of persons) it redirects to /accounts/login/ but the login page is actually under "/apis/accounts/login/"

@b1rger
Copy link
Contributor Author

b1rger commented Jun 4, 2024

when the url is at "/apis", you are not logged in and you click on lets say "Person" (to go to the list of persons) it redirects to /accounts/login/ but the login page is actually under "/apis/accounts/login/"

Ah, yes, indeed! But I'm not sure if we can make this run out of the box. /accounts/login is the default and it assumes that the user is including django.contrib.auth.urls under /accounts - which we do in most projects in addition to the apis shipped route /apis/accounts. So there are two approaches: a) use the apis shipped route and set your LOGIN_URL accordingly b) include django.contrib.auth.urls yourself

@sennierer
Copy link
Collaborator

ok, this solves the accounts urls problem (one or the other way), but still the root url of the sample_project doesnt serve anything. So why not just change the path from path("apis/", include("apis_core.urls", namespace="apis")) to path("", include("apis_core.urls", namespace="apis")) as suggested before. Like that just starting the docker container and pointing the browser to "http://localhost:8000" shows the default APIS landing page and the login works out of the box.
Its just the sample_project. Main goal should be to run right away.

@b1rger b1rger force-pushed the birger/602-dockerimage branch 2 times, most recently from 44573ff to acd2faf Compare June 5, 2024 11:42
@b1rger b1rger force-pushed the birger/602-dockerimage branch from acd2faf to 89fab2a Compare June 5, 2024 14:39
@b1rger b1rger requested a review from sennierer June 5, 2024 14:43
Copy link
Collaborator

@sennierer sennierer left a comment

Choose a reason for hiding this comment

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

Works nicely, apart from the fact that when properties are added, server needs to be restarted to actually get the relation forms. Given that this is another issue I opened #914

@b1rger b1rger merged commit 7a5d265 into main Jun 6, 2024
11 checks passed
@b1rger b1rger deleted the birger/602-dockerimage branch June 7, 2024 06:08
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.

Create a dockerimage
2 participants