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

Non interactive koel:init #886

Merged
merged 6 commits into from Jan 1, 2019
Merged

Non interactive koel:init #886

merged 6 commits into from Jan 1, 2019

Conversation

javier-lopez
Copy link
Contributor

@javier-lopez javier-lopez commented Dec 31, 2018

As per e1b68cc#diff-ac5db72ed5e3c5612246e80cb231cacf (Dec 3, 2017), the admin account is NOT set in the .env file and created during initial database seeding anymore, therefore it's required to run:

$ php artisan koel:init

In interactive mode to configure koel, such process can be automatized with expect but IMHO it could be better if it uses environment variables when available.

This PR re enable:

And add:

  • APP_MEDIA_PATH=/music

To be able to run $ php artisan koel:init in non interactive mode

@codecov-io
Copy link

codecov-io commented Dec 31, 2018

Codecov Report

Merging #886 into master will decrease coverage by 0.85%.
The diff coverage is 12.19%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #886      +/-   ##
============================================
- Coverage     68.57%   67.72%   -0.86%     
- Complexity      708      719      +11     
============================================
  Files           139      139              
  Lines          1750     1772      +22     
============================================
  Hits           1200     1200              
- Misses          550      572      +22
Impacted Files Coverage Δ Complexity Δ
app/Services/MediaSyncService.php 86% <ø> (ø) 34 <0> (ø) ⬇️
app/Console/Commands/InitCommand.php 6.1% <0%> (-1.17%) 36 <11> (+10)
app/Services/MediaMetadataService.php 61.29% <100%> (ø) 13 <1> (ø) ⬇️
app/Repositories/SongRepository.php 100% <100%> (ø) 4 <1> (ø) ⬇️
app/Repositories/AbstractRepository.php 70% <66.66%> (-7.78%) 6 <2> (+1)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e554448...397b8ff. Read the comment docs.

Copy link
Member

@phanan phanan left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, but I believe a better approach is to opt for the standard --no-interaction flag. Something like this:

private function inNoInteractionMode(): bool 
{
    return (bool) $this->getOption('no-interaction');
}

public function setUpAdminAccount(): void
{
    if ($this->inNoInteractionMode()) {
        // use .env creds, ready to explode if they're not provided.
    } else {
        // collect interactively, just like current
    }
}

Do you think you can tackle this?

@javier-lopez
Copy link
Contributor Author

Hi, thank you for the quick review, I just added the --no-interaction flag in javier-lopez@3ea4bc7

Copy link
Member

@phanan phanan left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great. A couple teeny-tiny changes and I believe we're all set!

if (!$name) {
$name = $this->ask('Your name');
if ($this->inNoInteractionMode()) {
$name = config('koel.admin.name');
Copy link
Member

Choose a reason for hiding this comment

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

Please don't align variable assignments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, undoing in next commit

@@ -121,26 +125,21 @@ private function setUpDatabase(): void
]);
}

private function inNoInteractionMode(): bool
{
return (bool) $this->option('no-interaction');
Copy link
Member

Choose a reason for hiding this comment

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

Is it option() or hasOption()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -49,6 +49,10 @@ public function handle(): void
$this->comment('Remember, you can always install/upgrade manually following the guide here:');
$this->info('📙 '.config('koel.misc.docs_url').PHP_EOL);

if ($this->inNoInteractionMode()) {
$this->info('Running in --no-interaction mode');
Copy link
Member

Choose a reason for hiding this comment

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

How about Running in no-interaction mode (without the double hyphen)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, modifying in next commit

return;
}

$this->error('The path '.$path.' does not exist or not readable. Skipping.');
Copy link
Member

Choose a reason for hiding this comment

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

We can return early here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just added a return statement after the error message

$this->error('The path '.$path.' does not exist or not readable. Skipping.');
return;

} else {
Copy link
Member

Choose a reason for hiding this comment

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

We don't need else if early return has been used (that's the whole point of early return here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

No, you don't need else because you have return'ed. Also, please add a new line before return statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I finally saw why it didn't required an else statement =P, I also tried to add new lines before returns, except when they are alone or around single lines.

@phanan
Copy link
Member

phanan commented Jan 1, 2019

@javier-lopez Sorry I kind of butchered your PR with some fixes on my own :)

@phanan phanan merged commit 7ba295e into koel:master Jan 1, 2019
@javier-lopez javier-lopez deleted the non-interactive-koel-init branch January 2, 2019 04:32
javier-lopez added a commit to javier-lopez/koel that referenced this pull request Aug 5, 2020
* Use ADMIN_* variables if available to create the admin account

* Add APP_MEDIA_PATH for media directory

* Use the standard --no-interaction flag to koel:init

* Undo variable aligment and code formatting

* Prefer early return over else, add new line before return statements

* Some fixes
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.

None yet

3 participants