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

Allow a different Apache port with APACHE_PORT #369

Merged
merged 1 commit into from
Jun 19, 2022

Conversation

devinmatte
Copy link
Contributor

@devinmatte devinmatte commented Jun 9, 2022

To deal with both #340 and #187, this PR allows for a new Environment variable APACHE_PORT to change the default port from 80 to something user defined. If unset, it remains using port 80

Fixes: #340

@ibennetch
Copy link
Member

I don't believe we need to do this for either fpm variant, because neither of those should include Apache.

Aside from that, this looks good to me, thanks for the contribution.

@devinmatte
Copy link
Contributor Author

devinmatte commented Jun 9, 2022

Makes sense, I did a quick pass again, hope it's good this time

Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

This contribution looks nice, thank you!

@devinmatte
Copy link
Contributor Author

Okay added an apachectl configtest to the statement. It looks like CI jobs aren't getting run because I'm a first time contributor, but from my read this looks good to go. Thanks for the quick review!

Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Thank you!
I will probably write a test for it, or maybe you can try to do it

@devinmatte
Copy link
Contributor Author

👋 I saw there were a few new commits in master. I updated the PR against master to see if that better passes CI

@williamdes
Copy link
Member

👋 I saw there were a few new commits in master. I updated the PR against master to see if that better passes CI

Thanks, I did some testing improvements to easily add a new test for this one

@williamdes williamdes self-assigned this Jun 19, 2022
@williamdes williamdes changed the title Allowing different apache ports Allow a different Apache port with APACHE_PORT Jun 19, 2022
williamdes added a commit that referenced this pull request Jun 19, 2022
@williamdes williamdes merged commit fee52fd into phpmyadmin:master Jun 19, 2022
williamdes added a commit that referenced this pull request Jun 19, 2022
@williamdes
Copy link
Member

I don't believe we need to do this for either fpm variant, because neither of those should include Apache.

I added 5ab888d on our update script to handle this. And updated the files: b936b8e

@williamdes
Copy link
Member

wave I saw there were a few new commits in master. I updated the PR against master to see if that better passes CI

Thanks, I did some testing improvements to easily add a new test for this one

Here is the test: 3c16423

Thank you for this contribution @devinmatte !

@williamdes
Copy link
Member

It is live on https://hub.docker.com/_/phpmyadmin !

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.

Change of Default Port 80
3 participants