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

Override default port using PORT environment variable #202

Closed
wants to merge 15 commits into from

Conversation

ardavanhashemzadeh
Copy link

This feature makes the container suitable for deployment in a Cloudfoundry environment.

ardavanhashemzadeh and others added 9 commits November 17, 2024 06:46
By allowing the PORT environment variable to override the default port our service listens on we can deploy to a Cloudfoundry environment.
Update README.rst with information about using the ``PORT`` environment variable.
Adding what's changing in the PR proposed to add the ``PORT`` environment variable.
…de-using-PORT-environment-variable

Adding ability to override default port using PORT environment variable
Build container and publish to ghcr.io upon successful CI workflow completion
@ardavanhashemzadeh
Copy link
Author

Updated to resolve conflict and bump version according to your latest commits, please let me know what else may be required to get this merged as I believe it can benefit others who wish to run this workload in a cloudfoundry or similar environment which require applications to accept traffic on configurable ports via environment variable.

@albertodonato
Copy link
Owner

Thanks for suggesting this change. I'm actually working to add support in prometheus-aioexporter for reading any config parameter from environment variables (using click), and also possibly reading it from .env files. This is going to be in 3.0.
Once I have that change finalized, I'll upgrade query-exporter to use it, which will allow passing settings, including the port, as env vars

@ardavanhashemzadeh
Copy link
Author

ardavanhashemzadeh commented Dec 14, 2024

Sounds like some exciting updates in 3.0, however looking at prometheus-aioexporter documentation it looks like it won't satisfy the reason for my raising this PR (see https://docs.cloudfoundry.org/devguide/deploy-apps/push-docker.html).

The changes in this PR have been deployed to and working in a production cf environment since the PR was opened, and while my team is happy to continue using the fork the point of the PR was to ensure continuity and prevent a project from forking into separate projects maintained by different teams if not necessary.

edit:
I just read the page I linked and looks like cf behavior in recent versions differs from that of older versions (now its aware of EXPOSE whereas in the versions of cf we have in production we had to use the PORT variable). So while the proposed change may no longer be required in up to date and bleeding edge versions of cloudfoundry many organizations run production environments which may not be the latest version, the proposed PR will help folks deploying so such environments.

@albertodonato
Copy link
Owner

Is the issue that with the 3.0 changes the port would be controlled by QE_PORT and not by PORT? If so, you can create a .env file which contains QE_PORT=${PORT} and that would make it work based on what's specified as PORT.

@ardavanhashemzadeh
Copy link
Author

The point of updating the scripts to accept "PORT" was to be able to use the published container as-is in CloudFoundry environments which are not aware of the EXPOSE parameter in the dockerfile. In other words, accepting the proposed changes will allow deployment of your base published image in a cloud native manner whereas your suggestion would require additional work. No point in beating a dead horse, closing the PR on basis of the conflicts introduced.

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.

2 participants