-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
improvement: add igniter installer #876
base: master
Are you sure you want to change the base?
Conversation
with this change, the installation instructions can be as simple as `mix igniter.install sentry --dsn "your_dsn"` For the release task change, a notice is emitted instructing the user to add the relevant task to their release process.
Oh, and you can test it yourself creating a new app: mix igniter.new my_app --with phx.new --install sentry@github:zachdaniel/sentry-elixir |
I forgot to explain the main endgame which is to put sentry as an option on the https://ash-hq.org installer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the direction!
@moduledoc false | ||
|
||
def short_doc do | ||
"Installs sentry. Requires igniter to be installed." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Installs sentry. Requires igniter to be installed." | |
"Configures Sentry. Requires igniter to be installed." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think per igniter conventions it would be better to leave this concept as "installs", but I can change it if you still think so after my other comment.
@@ -0,0 +1,188 @@ | |||
defmodule Mix.Tasks.Sentry.Install.Docs do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not installing Sentry, it's configuring it, right? Can we reflect that in the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the way igniter works is that when you run mix igniter.install package
, we look for a task defined by that package called <package>.install
that defines an igniter/2
function, so the task actually must be called Mix.Tasks.Sentry.Install
for that to work. In igniter we call these tasks an installer even in the cases that its already present in your mix.exs.
And from the end user's perspective, they don't typically call this task directly, they run mix igniter.install sentry
which will download and install the package.
For a preview of what I mean:
mix archive.install hex igniter_new
mix igniter.new --with phx.new my_app
cd my_app
mix igniter.install sentry@github:zachdaniel/sentry-elixir
# or one shot it with
mix igniter.new --with phx.new my_app --install sentry@github:zachdaniel/sentry-elixir
Co-authored-by: Andrea Leopardi <[email protected]>
Co-authored-by: Andrea Leopardi <[email protected]>
Co-authored-by: Andrea Leopardi <[email protected]>
Co-authored-by: Andrea Leopardi <[email protected]>
Co-authored-by: Andrea Leopardi <[email protected]>
Also realized I was missing some pieces here to make the installer idempotent, will have some new code to look at shortly. |
Something also to keep in mind is that for users who don't want to use igniter or can't for whatever reason, this shouldn't affect them at all given that it is an optional dependency. What we typically do is add a tabset to our installation guides, one for "with igniter (recommended)" and one for "manual". https://hexdocs.pm/ash/get-started.html#create-a-new-project |
with this change, the installation instructions can be as simple as
mix igniter.install sentry --dsn "your_dsn"
For the release task change, a notice is emitted instructing the user to add the relevant task to their release process.
All good if you'd rather not add this change, but the installation steps for sentry were a great candidate for an installer.
My next steps would be:
ash_sentry
package that composes this installer and adds the necessary integration.We can talk about Ash integration in a separate issue, just thought that I'd explain where I'm looking to go.