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

feat(optimus): add recipe for ckrack/optimus-bundle #946

Merged
5 commits merged into from
Jul 19, 2020

Conversation

ckrack
Copy link
Contributor

@ckrack ckrack commented May 25, 2020

Q A
License MIT

Add recipe for my bundle for jenssegers/optimus

Would be great to have a post-install-hook for recipes, as optimus could auto-configure with sensible values. Optimus needs some calculated prime numbers for instantiation. These should be different in each installation. To not fail during the composer install, we must use some default values. Of course we can use post-install.txt to tell people to exchange these defaults in the config/env. I'd like to add auto-configuration, though. There is no post-install-hook for bundles/recipes, so the only way I came up with is adding a script to auto-scripts with a composer-scripts-entry.
This script would replace the default values in the config with auto-generated ones. By replacing the defaults, this would only act once, even though the script itself will be invoked on every update/install.

I have two issues I'd like to discuss:

  1. Should I use only the config file, without any env vars preferably?
  2. Is the described way of auto-configuration via composer-scripts something we should do?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

  1. Should I use only the config file, without any env vars preferably?

If there are secrets that should not be in git, then yes, use environment variables. Just as you are doing.

  1. Is the described way of auto-configuration via composer-scripts something we should do?

Why is that needed? Because you want to help the user generating primes?

@ckrack
Copy link
Contributor Author

ckrack commented Jun 26, 2020

  1. Is the described way of auto-configuration via composer-scripts something we should do?

Why is that needed? Because you want to help the user generating primes?

Yes, the numbers can be auto-generated and are random by default, making each installation kind of unique.
Optimus' constructor throws an Exception when the values are invalid and this will lead to an error when auto-scripts are run after installation. So we need some defaults, but we want the user to change them (or automatically generate them).

@Nyholm
Copy link
Member

Nyholm commented Jun 28, 2020

The easiest way is to tell the user what to do in a post-install message.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

@ckrack
Copy link
Contributor Author

ckrack commented Jun 29, 2020

The easiest way is to tell the user what to do in a post-install message.

That's already in place. So, ready from my side.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

@ghost ghost merged commit e30cb80 into symfony:master Jul 19, 2020
This pull request was closed.
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