Skip to content

Merge back 4.8.2 changes #1665

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

Merged
merged 10 commits into from
Jan 5, 2022
Merged

Merge back 4.8.2 changes #1665

merged 10 commits into from
Jan 5, 2022

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Jan 4, 2022

I'm not sure if this is the right way to port 4-8 branch (currently 4.8.2) specific changes back to master, which are:

  • Versions
  • Changelog Entries

I thought about rebasing master on 4-8, which will yield a more natural git history. But it requires force pushing the master branch, which sounds like a big no-no to me.

And the other way is this: merging 4-8 back to master with this intermediate branch to resolve conflicts. It don't require any force-push but the commit history might look ugly (old commits marked as recently committed..etc.). This is the first time I do branch-based release so I'm not really sure what'd happen next.

Any idea? @sl0thentr0py

st0012 and others added 10 commits January 4, 2022 21:12
…ion (#1631)

* use prepended method instead of around_perform for activejob integration

since the sdk registers its around_perform as the "first" callback, it
needs to call all error handlers manually (which naturally should happen
later) to see if it should report the exception.

usually, this approach works well because if an exception will be
handled later, handling it ealier doesn't make much of a difference.

however, as described in #956, if there's another exception happened
inside one of the error handlers, it'll re-enter the error handler again
in here: https://github.com/rails/rails/blob/38cb5610b1/activejob/lib/active_job/execution.rb#l50-l51

Of course, it may be possible to handle such case by adding more rescue
block inside the `capture_and_reraise_with_sentry` method. But it'll
force the entire exception handling flow to be performed inside the
first `around_perform` block. And that's something we should avoid.

* Refactor ActiveJob integration

* Fix SendEventJob's rescue_from callback

* Update changelog
* Install yard

* Adopt yard format for documenting top-level APIs

* Fix YARD::Handlers::Ruby::MixinHandler in rake extension

```
[warn]: Load Order / Name Resolution Problem on Rake::Application:
-
Something is trying to call mixins on object Rake::Application before it has been recognized.
This error usually means that you need to modify the order in which you parse files
so that Rake::Application is parsed before methods or other objects attempt to access it.
-
YARD will recover from this error and continue to parse but you *may* have problems
with your generated documentation. You should probably fix this.
-

[error]: Unhandled exception in YARD::Handlers::Ruby::MixinHandler:
  in `lib/sentry/rake.rb`:31:

        31: Rake::Application.prepend(Sentry::Rake::Application)
```

* Mark private components

* Document configuration options with yard
* Remove unnecessary ActiveJob inclusion

* Update changelog
* Lock faraday to version 1.x

* Update changelog
@st0012
Copy link
Collaborator Author

st0012 commented Jan 5, 2022

@sl0thentr0py I'm going to merge this now in order to sync changes and patch out #1667

@st0012 st0012 merged commit 6be24ce into master Jan 5, 2022
@st0012 st0012 deleted the resolve-4-8 branch January 5, 2022 10:36
@sl0thentr0py
Copy link
Member

sry I just got up and saw this. I'm not sure about the process in this case either tbh.
I think having a few weird commits temporarily is fine in special cases.

@st0012
Copy link
Collaborator Author

st0012 commented Jan 5, 2022

@sl0thentr0py Because we use squash and merge, so all the weird commits seen in this PR were actually ignored as they don't introduce any real change. And we just need to remove them from the squashed commit message as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants