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

Onboarding Sail (docker) #247

Merged
merged 6 commits into from
Jun 7, 2024
Merged

Onboarding Sail (docker) #247

merged 6 commits into from
Jun 7, 2024

Conversation

gcavanunez
Copy link
Contributor

Highlights

  • Onboards sails deps and out of the box docker-compose.yml
  • Tweaks the vite config to play nice in none valet environments
  • Updates readme and gitignore

Doubts

  • Any particular reason https was strict? given sails lets the hostname up in the air, if we want to approach it from our end we could explore adding caddy to the mix
  • Given sail out of the box has strong opinions around the .env database config, wasn't 100% if it was worth highlighting on the .env.example or in the actual readme under the sail instructions

References

Copy link
Member

@driftingly driftingly left a comment

Choose a reason for hiding this comment

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

Looks good; only one small unrelated suggestion.
Thanks @gcavanunez!

vite.config.js Outdated Show resolved Hide resolved
@@ -0,0 +1,55 @@
services:
laravel.test:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super familiar with the ideal way to set up the Sail Docker compose file. Should this be ozzie.test? Or is laravel.test the correct config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I think the name is somewhat irrelant though I believe it's what's docker takes in as the name, thus definitely using the ozzie.test would be the way to go

Copy link
Contributor

Choose a reason for hiding this comment

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

Uhm, the Laravel service name is kinda hardcoded in the Sail bash script (see here) (it's the main service when we run things like sail artisan ...). Can we also add a APP_SERVICE="ozzie.test" to the .env.example file? This will get carried to the .env when someone is seting up the project.

@gcavanunez gcavanunez changed the title feat: Onboarding Sail (docker) Onboarding Sail (docker) Mar 1, 2024
Copy link
Contributor

@tonysm tonysm left a comment

Choose a reason for hiding this comment

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

Looks good. Left some comments, which should be addressed, but should good to merge IMO after those are handled.

.env.example Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
vite.config.js Outdated Show resolved Hide resolved
@gcavanunez gcavanunez force-pushed the feat/onboarding-sail branch from 63d8390 to 0ea7f72 Compare June 7, 2024 21:51
@gcavanunez gcavanunez force-pushed the feat/onboarding-sail branch from 0ea7f72 to 40986dc Compare June 7, 2024 22:55
@gcavanunez gcavanunez merged commit 51b083e into main Jun 7, 2024
4 checks passed
@gcavanunez gcavanunez deleted the feat/onboarding-sail branch June 7, 2024 23:04
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.

4 participants