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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: sentry.properties #191

Merged
merged 14 commits into from Jan 30, 2024
Merged

feat: sentry.properties #191

merged 14 commits into from Jan 30, 2024

Conversation

buenaflor
Copy link
Contributor

@buenaflor buenaflor commented Jan 17, 2024

馃摐 Description

Add sentry.properties for additional config options.

it tries to load sentry.properties first and if it doesn't exist it will fallback to pubspec.yaml

馃挕 Motivation and Context

Closes #42

馃挌 How did you test it?

Unit test
Manually

馃摑 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

馃敭 Next steps

  • Add sentry docs

Copy link
Contributor

github-actions bot commented Jan 17, 2024

Fails
馃毇 Please consider adding a changelog entry for the next release.
Messages
馃摉 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- sentry.properties ([#191](https://github.com/getsentry/sentry-dart-plugin/pull/191))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 馃毇 dangerJS against 443aa60

@buenaflor buenaflor marked this pull request as ready for review January 18, 2024 20:16
@krystofwoldrich
Copy link
Member

I don't have any comment on the code, that looks good to me.

I'm a bit concerned if this is not a breaking feature given the order we read the configs. If sentry.properties in the root of a Flutter project has not been used for anything till now, it should be fine.

I think the safer order would be to try first pubspec.yaml then sentry.properties then env. Although a bit more complex I would expect these two configs can coexist.

That would allow users to hide sensitive config to the properties file and then not commit it for example. Maybe not necessary.

@buenaflor
Copy link
Contributor Author

buenaflor commented Jan 19, 2024

Hm good point, at the very least it should be pubspec first and then sentry.properties.

Although a bit more complex I would expect these two configs can coexist.

Managing sensitive config can also be managed through env variables that can be added to sentry.properties afaik, gotta check if that works

Will go with the co-existing config route, will check out gradle plugin to see how they handle it

@buenaflor buenaflor marked this pull request as draft January 19, 2024 14:12
@buenaflor
Copy link
Contributor Author

@krystofwoldrich it seems like sentry.properties on the gradle plugin only uses org, project and auth-token so I think using sentry.properties as fallback and separating is the way to go. I guess we can adjust our approach based on demand but I think it should be good enough for now

@buenaflor buenaflor marked this pull request as ready for review January 23, 2024 17:04
Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

@buenaflor I agree, we can adjust based on demand, this solves the current issues and I don't expect high demand for combining the approaches.

@buenaflor buenaflor merged commit 1795b48 into main Jan 30, 2024
9 checks passed
@buenaflor buenaflor deleted the feat/sentry-properties branch January 30, 2024 08:52
@jtmuller5
Copy link

How are you supposed to create the sentry.properties file? I tried creating it as a json and as list of values like in pubspec.yaml but it is not recognized by the CLI

@buenaflor
Copy link
Contributor Author

key=value
# e.g
upload_debug_symbols=true

we'll add docs to the readme for that

  • Add docs for sentry.properties

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.

Support sentry.properties
3 participants