-
-
Notifications
You must be signed in to change notification settings - Fork 239
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
Add sentry_link for GraphQL #2338
base: main
Are you sure you want to change the base?
Conversation
thx!! I wonder how we are going to port the existing changelog since we symlink the main changelog in our packages and I don't necessarily want to remove the existing graphql changelog |
Yeah, I've wondered the same. Maybe keep it as E.g. add the following to the new changelog:
or something like that. |
thx, will get to this PR once the next sentry flutter release is out |
@buenaflor Are there any updates? |
sentry_link/lib/src/extension.dart
Outdated
/// Extension for [SentryOptions] | ||
extension InAppExclueds on SentryOptions { | ||
/// Sets this library as not in-app frames, to improve stack trace | ||
/// presentation in Sentry. | ||
void addSentryLinkInAppExcludes() { | ||
addInAppExclude('sentry_link'); | ||
} | ||
} |
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 could be added to the default ones in the sentry
package instead. However, I would prefer that to be done in a follow-up PR, to keep this PR essentially just as the take-over.
@buenaflor or @vaind any chances of moving forward with this? :D By download count if would be on the 6th place of all Sentry packages in the Dart ecosystem |
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.
can we change the folder namesentry_link
-> link
? to keep in line with the other namings
and
craft.yml still needs an entry link
for the pub dev section
I will do the registry entry manually later (I need to add it to the sentry registry repo first after an initial release)
Sure
I'll take a look at it. |
Maybe we can also setup a simple CI for running the tests similar to hive.yml nvm we can do this after the pr |
@kahest do we need to look out for anything specific when transferring external repos to ours? So far we:
|
Handing over the package name on pub.dev still needs to be coordinated too. |
for my understanding: since we'll use the same pubspec config, any new release will just update the sentry_link package directly right? |
Yeah, but I need to hand over the ownership of the "name" on pub.dev. Otherwise the upload will just fail |
what about this
wdyt? |
Sounds good. I'm happy to jump on call or something for the package transfer on pub. I guess that's easier than sending messages back and forth |
.craft.yml
Outdated
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.
sry, let's remove the craft targets for now, I'll do this manually after I've set up CI and so on
otherwise it will try to upload the package already when we run another release (which we might soon)
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.
Done
📜 Description
Resolves #1126
💡 Motivation and Context
Context: #1126 (comment)
💚 How did you test it?
It's already used widely in production without any reported issues
📝 Checklist
sendDefaultPii
is enabled🔮 Next steps
sentry_link
to justlink
orgraphql
?Open questions
What to do with the changelog? All other packages reuse the same oneI'm happy to discuss this via Discord, Twitter, E-Mail, or whatever :D