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

[WIP] Create Install Command #118

Draft
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

AlexJump24
Copy link
Contributor

@AlexJump24 AlexJump24 commented Oct 14, 2024

Currently WIP as need to prompt for the configuration values if the user says yes to configuring Cachet.

One thing I'm not 100% sure the best approach for is configuring a secondary connection, incase you want to the migrations separate to your main database. Something I'd probably always want to do given the potential for table name clashes etc.

I wasn't sure if either to define getConnectionName() on each model which I think is the approach Laravel Passport takes, or extends the base model and defining on there and having all models extend the internal base model. Or there is probably an even better idea 😄

I've also moved the database settings from the seeder to the migration as it was kind of duplicating the logic, if this is not ok I can revert. Same with moving things from the testbench.yaml as felt it may as well be in the install command now other than the Sqlite file which would be needed for Testbench.

Once the configuration values are set up I am not sure what else would then be needed as its a bit different now given its composer required into applications rather than a standalone Laravel application.

Hopefully this is all ok so far!

@jbrooksuk jbrooksuk force-pushed the feature/issue-104-install-command branch from bcc4b52 to 760bd43 Compare October 15, 2024 09:51
@jbrooksuk jbrooksuk linked an issue Oct 15, 2024 that may be closed by this pull request
@jbrooksuk
Copy link
Member

@AlexJump24 I think some of the options we ask for don't need to be required. For example:

  • About
  • Refresh Rate

And maybe the attribute can have two properties; required and default?

src/Settings/Attributes/Description.php Outdated Show resolved Hide resolved
src/Settings/Attributes/Description.php Outdated Show resolved Hide resolved
src/Commands/InstallCommand.php Outdated Show resolved Hide resolved
@AlexJump24
Copy link
Contributor Author

AlexJump24 commented Oct 18, 2024

sure 👍🏼 @jbrooksuk

Did you have any preference on the following for supporting a separate connection.

I wasn't sure if either to define getConnectionName() on each model which I think is the approach Laravel Passport takes, or extends the base model and defining on there and having all models extend the internal base model. Or there is probably an even better idea

@jbrooksuk
Copy link
Member

jbrooksuk commented Oct 19, 2024

Hmm, I think I'd use the method that Pulse uses, where it has a connection config which I believe fallsback to the default.

https://github.com/laravel/pulse/blob/5dd3cd257305c796900a69d3307fea3de1425813/config/pulse.php#L59-L66

@jbrooksuk
Copy link
Member

Hey @AlexJump24, how's this going?

@jbrooksuk jbrooksuk force-pushed the feature/issue-104-install-command branch from 44bb282 to c628bc5 Compare November 30, 2024 15:23
@AlexJump24
Copy link
Contributor Author

Hi @jbrooksuk sorry I totally missed this message, I have been unfortunately very busy so have been unable to look into this, I am hoping with the Xmas break coming up that I'll have some time to address the latest feedback for you 🙂

@jbrooksuk
Copy link
Member

@AlexJump24 no problem! Enjoy Christmas and I hope to see your future contributions 🫶

@jbrooksuk
Copy link
Member

By the way @AlexJump24, this command can also optionally call cachet:make:user to finish the setup process 👌

@AlexJump24
Copy link
Contributor Author

Thanks @jbrooksuk I hope you are having a good one too. I have added the make user command in, just have a slight confusion regarding what you mentioned for Pulse, as yeah that implements the config like I have put in place here https://github.com/AlexJump24/core/blob/feature/issue-104-install-command/config/cachet.php#L119 but to my understanding I'd either need to put this on every model overloading the getConnection method or get each model to extend a common interface which has this method which references the config with a fallback to the original connection. Unless of course there's a pattern deeper into Pulse I need to follow but my initial thoughts were they weren't really relevant.

Once I have got this understanding clear I should be able to quickly sort this out for you 🙂

@jbrooksuk
Copy link
Member

@AlexJump24 you can ignore me, what you have here is great! Happy for me to merge this?

Would love to also get this documented as an alternative way to install Cachet.

@AlexJump24
Copy link
Contributor Author

@AlexJump24 you can ignore me, what you have here is great! Happy for me to merge this?

Would love to also get this documented as an alternative way to install Cachet.

@jbrooksuk I've made a few more changes, testing again installed in another Laravel project. I still believe we need a way to tell the cachet models that they belong on the Cachet connection though. Whether that is overloading the getConnectionName method in each model, getting each model to use a trait which overloads the method or having an abstract base class for all of them to extend.

If not although I pass the connection for the migrations in the install command the models are going to try and use the connection of the Laravel application they are installed in.

I've also had to remove the truncate methods from the seeder I hope this is ok as it truncated my users table, and there is already a db:wipe on the testbench.yaml

@jbrooksuk
Copy link
Member

@AlexJump24 right, I'm with you now! Let's add a trait that's like ManagesConnections or something. That can just be used to override the connection with the config (or fallback to default).

@jbrooksuk
Copy link
Member

Perfect, @AlexJump24!

@AlexJump24
Copy link
Contributor Author

Perfect, @AlexJump24!

Just one small other issue now is getting Cachets routes to work on its own DB connection if set, I have a few ideas but this is quite evolving to become its own issue 😄 If using your own connection i think the users table for Cachet would need to be independent for Cachet which I think makes sense, due to the relationships etc but then loading the appropriate database connection would then need to be handled at route level. If you have any ideas either or if this needs splitting out into its own things to get this install command in quicker let me know.

@jbrooksuk
Copy link
Member

Hmm, yeah. That's a tricky one. I think that if you set a custom database connection then all models should use it, including users.

How we go about making sure that all of Cachet uses this (since we use raw DB queries too) is really a tedious task. We need to update all DB calls to set the connection first.

Maybe we park this for now while we think on it? I'll probably come back to this PR myself and have a play with it soon.

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.

Install Command
2 participants