Skip to content

Conversation

@rafaeljesus
Copy link
Contributor

Allow read dsn system values for deployment

Closes #107

I kindly ask you to merge it asap as I have a application running in production waiting this feature,

Thanks

The event is dropped if it all retries fail.
"""
@spec send_event(%Event{}) :: {:ok, String.t} | :error
def send_event(%Event{} = event) do
Copy link
Contributor Author

@rafaeljesus rafaeljesus Jan 19, 2017

Choose a reason for hiding this comment

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

Basically we should be injecting dependences in function signature so we can easily test and move private functions to its own module which we can also test independently so it could be like:

def send_event(%Event{} = event, dsn_parser \\ DsnParser) do
{endpoint, public_key, secret_key} = dsn_parser.parse!
# ....
end

Unfortunately I don't have time to implement it yet, but we can refactor it soon

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that that would be necessary. This kind of pattern would usually come along with specifying a Behaviour, which might be overkill at this point 🙂

end)
end

def dsn_env do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it is public for testing, this should be moved to another module DsnParser and be injected on send_event public function

@mitchellhenke
Copy link
Contributor

I can start reviewing this at some point today, but if you need it ASAP, you can reference your fork in mix with: {:sentry, git: "git://github.com/rafaeljesus/sentry-elixir.git", branch: "read-dsn-from-system"}

@rafaeljesus
Copy link
Contributor Author

@mitchellhenke Thanks!

end

def dsn_env do
case Application.get_env(:sentry, :dsn) do
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should stay as Application.fetch_env!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right!

@rafaeljesus
Copy link
Contributor Author

@mitchellhenke I also just realized that I need to do the same on Mix.Tasks.Sentry.SendTestEvent

@mitchellhenke
Copy link
Contributor

mitchellhenke commented Jan 19, 2017

@rafaeljesus It might be easier to have something like Sentry.Client.get_dsn! that fetches from sys/env and parses. Then it can just be called in Sentry.Tasks.Sentry.SendTestEvent, Sentry.Client.send_event/1, and the tests. What do you think?

@rafaeljesus
Copy link
Contributor Author

@mitchellhenke mate I did the changes you requested may you take a 👀 again?

@rafaeljesus
Copy link
Contributor Author

Also execute the send_test_event manually with my sentry account and works like a charm

@mitchellhenke mitchellhenke merged commit ef0913f into getsentry:master Jan 21, 2017
@mitchellhenke
Copy link
Contributor

Thanks for the contribution!

@rafaeljesus rafaeljesus deleted the read-dsn-from-system branch January 21, 2017 15:53
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