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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate Sentry to 15.0 #2386

Closed
wants to merge 22 commits into from
Closed

Conversation

aisopuro
Copy link
Contributor

This adds fixes to the migration started in #2376. Most notably fixes the tests to actually run.

naglis and others added 22 commits July 19, 2022 00:57
* [ADD] sentry module

* [FIX] updated sentry module according to PR comments
- [FIX] sentry: fixes missing `raven` library preventing loading of modules
- [FIX] 2to3 script on py file
- [FIX] add requirements.txt
sentry: It is not always possible to read commit information from
a production environment. In those cases it is useful to be able
to set a release version manually.

[UPD] Update sentry.pot
[UPD] Update sentry.pot
Update translation files

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: server-tools-14.0/server-tools-14.0-sentry
Translate-URL: https://translation.odoo-community.org/projects/server-tools-14-0/server-tools-14-0-sentry/

[UPD] README.rst

[UPD] Update sentry.pot
The following warning is fixed:

    DeprecationWarning: Using or importing the ABCs from 'collections' instead of
    from 'collections.abc' is deprecated since Python 3.3, and in 3.9 it will stop
    working
Allow using `sentry_release` or `sentry_odoo_dir` in the Odoo
configuration file.

Previously, the `sentry_odoo_dir` was never actually respected. It would
always be overridden by `sentry_release`. Even if `sentry_release` is
not set, it will use an empty value instead of using `sentry_odoo_dir`
to find the Git commit hash.

After this commit, the `sentry_release` parameter still takes
precedence. However, if `sentry_release` is not set and
`sentry_odoo_dir` is set, then `sentry_odoo_dir` will be used to find
the appropriate Git commit hash, which will be used as the `release`
value.

Both cases are covered by the added unit tests.
Changes to Odoo's underlying test functionality make it so that bare
unittest classes do not get run, as they are missing tags and expected
attributes.

Subclass odoo.tests.common.BaseCase instead in order to make Odoo run
these tests.
At some point the underlying sanitation logic changed to no longer
do sanitation if the key in the cookie was not followed by '='.
@aisopuro aisopuro marked this pull request as ready for review August 10, 2022 14:37
@RaveendarU
Copy link

@aisopuro aisopuro Any specific reason for using old 'raven' package for Sentry(version:15) instead of 'sentry-sdk' package which we used in previous versions.

@aisopuro
Copy link
Contributor Author

aisopuro commented Oct 10, 2022

Unfortunately @RaveendarU just lack of time. I don't have the bandwidth to refactor/rebuild the module to use the new SDK, and we're succesfully running this code as is.

I agree it would be better to move to the new SDK since Sentry themselves recommend it, but unfortunately I won't have time to implement that in the foreseeable future. If someone knows that it is super-easy to do and has a fast test iteration setup ready, I'm more than willing to let this PR be stolen from me 😄

OK, now I actually read your comment and see that you are referring to the fact that the 14 version already uses the new SDK. 🤦

No, there's no reason for that other than oversight: we either did not notice that code when we started migrating this, or it wasn't available. We should most likely simply re-do the migration basing it off the 14 version. Though again, I don't have a lot of bandwidth. I'll see what I can come up with.

@thomaspaulb
Copy link
Contributor

I'm thinking it could be as simple as rebasing this PR's branch on the latest 14.0, to which the sentry_sdk refactor was merged a couple of days ago: #2254

@aisopuro aisopuro mentioned this pull request Oct 17, 2022
@aisopuro
Copy link
Contributor Author

Alright @thomaspaulb I've opened a third attempt at migration: #2428 . See the notes on that PR for explanation. I'll close this one as unnecessary.

@aisopuro aisopuro closed this Oct 17, 2022
@thomaspaulb
Copy link
Contributor

Thanks for the work! You could have force-pushed it over here as well, but it doesnt matter, I'll review the other one.

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.