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: enable v1 root drf browsable and fix staticfiles #945

Merged
merged 9 commits into from
Jul 1, 2024

Conversation

Alex-Izquierdo
Copy link
Collaborator

@Alex-Izquierdo Alex-Izquierdo commented Jun 18, 2024

Enable the v1 root for DRF browsable and adds the missing collecstatic step.
It requires updates in the installer/operator/ui_image to run the colllecstatic command and serve those static files.
Jira: https://issues.redhat.com/browse/AAP-25508
Screenshot from 2024-06-18 18-42-08

@Alex-Izquierdo Alex-Izquierdo requested a review from a team as a code owner June 18, 2024 16:44
@Alex-Izquierdo Alex-Izquierdo marked this pull request as draft June 18, 2024 16:59
@Alex-Izquierdo Alex-Izquierdo changed the title feat: enable v1 root drf browsable and fix staticfiles WIP feat: enable v1 root drf browsable and fix staticfiles Jun 18, 2024
@Alex-Izquierdo Alex-Izquierdo marked this pull request as ready for review June 18, 2024 17:18
@Alex-Izquierdo Alex-Izquierdo changed the title WIP feat: enable v1 root drf browsable and fix staticfiles feat: enable v1 root drf browsable and fix staticfiles Jun 18, 2024
@Alex-Izquierdo Alex-Izquierdo marked this pull request as draft June 18, 2024 17:24
@Alex-Izquierdo Alex-Izquierdo changed the title feat: enable v1 root drf browsable and fix staticfiles WIP feat: enable v1 root drf browsable and fix staticfiles Jun 18, 2024
@Alex-Izquierdo Alex-Izquierdo force-pushed the enable-drf-root branch 3 times, most recently from 0e3bc73 to 6489d94 Compare June 18, 2024 18:21
@Alex-Izquierdo Alex-Izquierdo marked this pull request as ready for review June 18, 2024 18:27
@Alex-Izquierdo Alex-Izquierdo changed the title WIP feat: enable v1 root drf browsable and fix staticfiles feat: enable v1 root drf browsable and fix staticfiles Jun 18, 2024
rooftopcellist
rooftopcellist previously approved these changes Jun 19, 2024
mkanoor
mkanoor previously approved these changes Jun 19, 2024
@Alex-Izquierdo
Copy link
Collaborator Author

Don't merge this for now, please. I want to sync the required changes in other places like the installer to avoid broken builds.

@Alex-Izquierdo Alex-Izquierdo dismissed stale reviews from mkanoor and rooftopcellist via 28683db June 19, 2024 17:18
Signed-off-by: Alex <aizquier@redhat.com>
Signed-off-by: Alex <aizquier@redhat.com>
Copy link
Member

@Dostonbek1 Dostonbek1 left a comment

Choose a reason for hiding this comment

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

Looking at the JIRA ticket expected behavior, this PR doesn't seem to consider the following case:

/api/eda should provide version options for the API

@Dostonbek1
Copy link
Member

Dostonbek1 commented Jun 27, 2024

Another minor comment, it seems a little redundant to have the hostname before every endpoint listed in v1 root view, e.g. http://localhost:8000/api/eda/users/me/. Since it is obvious what the hostname is, maybe we can drop it; it would make the endpoints more readable too. Thoughts?

@Alex-Izquierdo
Copy link
Collaborator Author

Looking at the JIRA ticket expected behavior, this PR doesn't seem to consider the following case:

/api/eda should provide version options for the API

It is out of the scope of this PR. As you can see in the description of the jira, only includes the v1/

@Alex-Izquierdo
Copy link
Collaborator Author

Another minor comment, it seems a little redundant to have the hostname before every endpoint listed in v1 root view, e.g. http://localhost:8000/api/eda/users/me/. Since it is obvious what the hostname is, maybe we can drop it; it would make the endpoints more readable too. Thoughts?

It seems to me a valid point. I think it is due to the way I generate automatically all the links. I was trying to merge this PR in sync with other required changes to avoid breaking build. New changes in the PR would complicate this. Do you agree to merge as it is and later address the host in a later PR?

@Alex-Izquierdo Alex-Izquierdo merged commit c1fca5a into ansible:main Jul 1, 2024
4 checks passed
@Alex-Izquierdo Alex-Izquierdo deleted the enable-drf-root branch July 1, 2024 15:01
jshimkus-rh pushed a commit to jshimkus-rh/eda-server that referenced this pull request Jul 1, 2024
Signed-off-by: Alex <aizquier@redhat.com>
Co-authored-by: Madhu Kanoor <mkanoor@redhat.com>
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.

6 participants