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

Add check for Unit custom certs #321

Merged
merged 2 commits into from
Apr 23, 2024

Conversation

jbxonline
Copy link
Contributor

I'm not convinced we need the first check on line 157 at all as I don't know how a cert would ever get into the config directory at this stage. However, maybe it is useful if someone is building a custom image and copying their SSL directly into the config directory. So I've left it in place.

However, we previously then went straight to creating the self signed cert, without ever actually taking a look inside /etc/ssl/private to see if there was a custom cert in there. So this PR basically adds that check to use a custom cert if provided before moving on to generating a self signed.

@jaydrogers jaydrogers self-requested a review April 23, 2024 12:26
@jaydrogers jaydrogers merged commit 2e22cc7 into serversideup:main Apr 23, 2024
@jaydrogers
Copy link
Member

This looks great!!

I'm not convinced we need the first check on line 157 at all as I don't know how a cert would ever get into the config directory at this stage. However, maybe it is useful if someone is building a custom image and copying their SSL directly into the config directory. So I've left it in place.

I intentionally put that in there because I was running into weird things with container caching.

However, we previously then went straight to creating the self signed cert, without ever actually taking a look inside /etc/ssl/private to see if there was a custom cert in there. So this PR basically adds that check to use a custom cert if provided before moving on to generating a self signed.

I love how you did this and I apologize for missing that earlier. I was writing a lot of code really fast and learning something at the time of creating that script. It's a dangerous combo 🤣

Merged into our "dev" images

This has been merged and is building here: https://github.com/serversideup/docker-php/actions/runs/8800388318

They will be made available here: https://hub.docker.com/r/serversideup/php-dev

Once that job is done, let me know that everything works for you with the dev image 👍

I greatly appreciate your contribution and will gladly accept more PRs in the future!

@jbxonline
Copy link
Contributor Author

Thanks for the quick merge! Really helpful, hopefully I can ship a beta today pulling from your dev repo rather than my personal repos :)

I intentionally put that in there

Perfect - if it's needed - it's needed 👍🏻

Once that job is done, let me know that everything works for you with the dev image

Yeah, don't you just hate how slow docker builds are in ci environments!
Anyways, once it is built, I'll test it. Hopefully later this week I will get some time to test some more features of this variation and I'll let you know if I spot any issues. Also, if anyone raises an issue with the unit variation and you're short on time, feel free to tag me and I'll take a look for you.

@jaydrogers
Copy link
Member

Thanks a ton!

Yeah, don't you just hate how slow docker builds are in ci environments!

Yes, I tried to solve this yesterday and failed 😆 #316

Images should be built, so keep me posted if everything works well 👍

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