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

BAH-3545 | Add. Favicon To Home Page #913

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

BAH-3545 | Add. Favicon To Home Page #913

wants to merge 1 commit into from

Conversation

rahu1ramesh
Copy link
Contributor

JIRA -> BAH-3545

Copy link
Contributor

@umair-fayaz umair-fayaz left a comment

Choose a reason for hiding this comment

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

I am noticing that we have a bit of redundancy with this index.html file being present in both the proxy service and the bahmni-web service. I believe we can streamline this process and avoid maintaining the same file in multiple places.

Instead of duplicating the index.html file, I suggest we centralize it within the bahmni-web service. Since the bahmni-web service is already serving this file, we can remove it from the proxy container.

To ensure that requests to the root path "/" are directed to bahmni-web, we can update the proxy configuration to include a ProxyPass rule for "/" pointing to bahmni-web.

Let me know if you have any thoughts or suggestions on this approach.

@mohan-13
Copy link
Member

I am noticing that we have a bit of redundancy with this index.html file being present in both the proxy service and the bahmni-web service. I believe we can streamline this process and avoid maintaining the same file in multiple places.

Instead of duplicating the index.html file, I suggest we centralize it within the bahmni-web service. Since the bahmni-web service is already serving this file, we can remove it from the proxy container.

To ensure that requests to the root path "/" are directed to bahmni-web, we can update the proxy configuration to include a ProxyPass rule for "/" pointing to bahmni-web.

Let me know if you have any thoughts or suggestions on this approach.

@umair-fayaz This index.html from bahmni-web is used only by k8s environment where proxy image is not used and in docker compose based deployments proxy container serves and thats the reason for the redundancy

@umair-fayaz
Copy link
Contributor

I am noticing that we have a bit of redundancy with this index.html file being present in both the proxy service and the bahmni-web service. I believe we can streamline this process and avoid maintaining the same file in multiple places.
Instead of duplicating the index.html file, I suggest we centralize it within the bahmni-web service. Since the bahmni-web service is already serving this file, we can remove it from the proxy container.
To ensure that requests to the root path "/" are directed to bahmni-web, we can update the proxy configuration to include a ProxyPass rule for "/" pointing to bahmni-web.
Let me know if you have any thoughts or suggestions on this approach.

@umair-fayaz This index.html from bahmni-web is used only by k8s environment where proxy image is not used and in docker compose based deployments proxy container serves and thats the reason for the redundancy

Yes, @mohan-13 . While I understand that the index.html from bahmni-web is specifically used in Kubernetes environments where the proxy image is not utilized, my suggestion was aimed at streamlining the process even for docker-compose based deployments by loading the index.html from bahmni-web.
Eliminating redundancy in this manner could simplify maintenance. Considering that logos and icons for this index.html are fetched from bahmni-web, the proxy already relies on bahmni-web for these assets. So, integrating the index.html loading from bahmni-web in docker-compose setups could enhance consistency and reduce duplication. Do you foresee any challenges with this approach?

@mohan-13
Copy link
Member

I am noticing that we have a bit of redundancy with this index.html file being present in both the proxy service and the bahmni-web service. I believe we can streamline this process and avoid maintaining the same file in multiple places.
Instead of duplicating the index.html file, I suggest we centralize it within the bahmni-web service. Since the bahmni-web service is already serving this file, we can remove it from the proxy container.
To ensure that requests to the root path "/" are directed to bahmni-web, we can update the proxy configuration to include a ProxyPass rule for "/" pointing to bahmni-web.
Let me know if you have any thoughts or suggestions on this approach.

@umair-fayaz This index.html from bahmni-web is used only by k8s environment where proxy image is not used and in docker compose based deployments proxy container serves and thats the reason for the redundancy

Yes, @mohan-13 . While I understand that the index.html from bahmni-web is specifically used in Kubernetes environments where the proxy image is not utilized, my suggestion was aimed at streamlining the process even for docker-compose based deployments by loading the index.html from bahmni-web. Eliminating redundancy in this manner could simplify maintenance. Considering that logos and icons for this index.html are fetched from bahmni-web, the proxy already relies on bahmni-web for these assets. So, integrating the index.html loading from bahmni-web in docker-compose setups could enhance consistency and reduce duplication. Do you foresee any challenges with this approach?

I agree. But not sure how all the redirections would behave when the root / is proxied to bahmni-web. We need to validate all the redirection scenarios before making the change

@umair-fayaz
Copy link
Contributor

I am noticing that we have a bit of redundancy with this index.html file being present in both the proxy service and the bahmni-web service. I believe we can streamline this process and avoid maintaining the same file in multiple places.
Instead of duplicating the index.html file, I suggest we centralize it within the bahmni-web service. Since the bahmni-web service is already serving this file, we can remove it from the proxy container.
To ensure that requests to the root path "/" are directed to bahmni-web, we can update the proxy configuration to include a ProxyPass rule for "/" pointing to bahmni-web.
Let me know if you have any thoughts or suggestions on this approach.

@umair-fayaz This index.html from bahmni-web is used only by k8s environment where proxy image is not used and in docker compose based deployments proxy container serves and thats the reason for the redundancy

Yes, @mohan-13 . While I understand that the index.html from bahmni-web is specifically used in Kubernetes environments where the proxy image is not utilized, my suggestion was aimed at streamlining the process even for docker-compose based deployments by loading the index.html from bahmni-web. Eliminating redundancy in this manner could simplify maintenance. Considering that logos and icons for this index.html are fetched from bahmni-web, the proxy already relies on bahmni-web for these assets. So, integrating the index.html loading from bahmni-web in docker-compose setups could enhance consistency and reduce duplication. Do you foresee any challenges with this approach?

I agree. But not sure how all the redirections would behave when the root / is proxied to bahmni-web. We need to validate all the redirection scenarios before making the change

I agree with your concern about redirection behaviour when proxying the root path to bahmni-web. Testing all redirection scenarios is definitely crucial.

To get a head start, I quickly tested it on a codespace environment. Here's what I did and observed:

  -  Removed `index.html` from proxy image.
  - Added a new ProxyPass rule: `ProxyPass / http://bahmni-web:8091/`

Observed successful loading of index.html from bahmni-web, along with proper loading of all assets. Redirects under /bahmni/home and /openmrs functioned as expected. My environment didn't have other modules running, so I couldn't verify their redirection behavior. However, the core functionality seems to be working well.

Screenshots:
Screenshot 2024-04-23 at 5 12 31 PM
Screenshot 2024-04-23 at 5 16 50 PM
Screenshot 2024-04-23 at 5 17 02 PM

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.

3 participants