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 Laravel sail for dev container #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add Laravel sail for dev container #47

wants to merge 1 commit into from

Conversation

eddiezane
Copy link
Member

This PR adds Laravel sail to add a dev container to the project.

It also adds database seeds for local development.

We will follow up with a development guide in another PR.

Signed-off-by: Eddie Zaneski <[email protected]>

Co-authored-by: Casey Hunger <[email protected]>
@eddiezane eddiezane requested a review from Jnesselr January 14, 2024 00:33
Comment on lines +35 to +38
if (config('app.env') == 'local') {
Log::warning('APP_ENV is local, skipping mTLS check for Slack');
return true;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a better way to do this? ngrok has an mTLS module but it's kinda a pain.

Copy link
Member

Choose a reason for hiding this comment

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

App::environment('local') for the command to check if you're running locally. Personally, if you're testing locally there's not too much reason to verify the Slack cert unless you leave that tunnel open.

Copy link
Member

@Jnesselr Jnesselr left a comment

Choose a reason for hiding this comment

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

I can't verify the docker compose as I don't want to add docker to my new laptop and test all of that, but it seems reasonable.

Comment on lines +35 to +38
if (config('app.env') == 'local') {
Log::warning('APP_ENV is local, skipping mTLS check for Slack');
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

App::environment('local') for the command to check if you're running locally. Personally, if you're testing locally there's not too much reason to verify the Slack cert unless you leave that tunnel open.


];

foreach($memberships as [$customerId, $planIds]) {
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, this isn't a customer id but it's a full blown customer. Laravel will translate the model to the id for you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's what I expected too, but when I used it as a customer object it failed, and subsequent logging suggested it was just returning an ID. IDK what I was doing wrong but this works. How do you recommend we change it?

}
Log::info('CustomerSeeder: Seeding Customers and UserMemberships.');

$superuser = Customer::factory()
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of the super user, out of curiosity. It just seems like they've been given all the training here so I'm wondering if that means it must be updated whenever we hardcode a new membership plan.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The super user is a user that you can use to not have to worry about not having permissions for things. It's a good starting point to discovering/testing functionality without being restricted by lack of permissions. If you want to test/add functionality that it doesn't have, yes, you'll need to update it. Seed maintenance is a thing, but it shouldn't be a big lift. If someone wants to automate it further, that would certainly be welcome. This is just a starting point so that you don't have to go craft the first user in a blank database and figure out what all it needs in order to be functional. Seeds also serve as quick-start examples of what data affects a change in functionality.

@chungl
Copy link
Collaborator

chungl commented Jan 15, 2024

@eddiezane can you add the cliffnotes of the development guide here? I haven't been able to run this yet and I don't really want to go read more documentation right now to figure it out. Presumably I need to install php and composer, run composer install, and then run some sail commands to start this version?

@Jnesselr
Copy link
Member

@eddiezane This PR is out of date in that it's not merge-able due to the lock file.

I'm wondering if we should split out the factories work from the Laravel Sail work. What are your thoughts? Personally, I've been using Laravel Herd for my environment, but I only see that available on Windows/Mac.

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.

3 participants