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

define nginx ports to be overridable #1536

Conversation

codePau
Copy link
Contributor

@codePau codePau commented Sep 19, 2022

Issue #1300

Copy link
Member

@metasoarous metasoarous left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this!

This is great, but I would like for the defaults to match current behavior before merging.

If you have time, it would also be great to have this documented, either in the main README, or in https://github.com/compdemocracy/polis/blob/dev/docs/configuration.md. Having just gone through this yourself, please let us know where you would have found this option most helpful.

Thanks again!

.env Outdated
@@ -1 +1,2 @@
TAG=dev
HTTP_PORT=2000
Copy link
Member

Choose a reason for hiding this comment

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

This is great! But can we please leave the default HTTP port to 80?

I think it should be possible to set a different default in the docker-compose.dev.yml overlay if you want to have a dev-time port other than 80. However, I'd like for that default to be 5000, since this has been the historical default for the dev tooling.

@metasoarous
Copy link
Member

I wanted to use this change, so went ahead and fixed it up, and am merging.

Thanks again!

@metasoarous metasoarous merged commit 52e983a into compdemocracy:dev Sep 21, 2022
@codePau
Copy link
Contributor Author

codePau commented Sep 25, 2022

Thanks @metasoarous! Yes this line was just for testing purposes. Glad that you moved forward with it and it's now merged.

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.

None yet

2 participants