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 #2428

Merged
merged 32 commits into from
Oct 29, 2022
Merged

Conversation

aisopuro
Copy link
Contributor

This PR replaces #2386:

  • This branch has been rebuilt on top of 14.0 in order to include the recently merged switch to using sentry_sdk instead of raven.
  • I "stole credit" for the original migration from the previous author, but considering it was only changing the version in the manifest, this seems like a minor issue
  • The previous PR included commits that fixed tests to run: these were already fixed in 14.0, and so those commits are not necessary in this PR.
  • This is a new PR because I need the old branch to exist as-is as it is being referenced in some projects, and I need to verify it is safe for them to suddenly change their pip-requirements.

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

2 tests are failing, but they are not in sentry. I'll try force-pushing to re-trigger the pipelines, in the hopes that helps.

@aisopuro
Copy link
Contributor Author

Failing tests appear to be in letsencrypt. Judging by the exception the unit test is actually contacting LetsEncrypt, and getting an error about assigning certificates to *.example.com:

acme.messages.Error: urn:ietf:params:acme:error:rejectedIdentifier :: The server will not issue certificates for the identifier :: Error creating new order :: Cannot issue for "*.example.com": The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy

Possibly the policy has changed since the tests were written.

There are no errors in the 14.0 branch, I'll see if there are fixes in there.

@aisopuro
Copy link
Contributor Author

There are no changes to letsencrypt between this branch and 14.0. I don't know what to do.

@janverb and @NL66278 , you seem to be the ones who introduced one of the tests that is now failing: https:/OCA/server-tools/blob/14.0/letsencrypt/tests/test_letsencrypt.py#L79

Do either of you have an idea of what might be going wrong here? Judging by the error, it almost seems like we're getting a response from actual LetsEncrypt, despite it seeming like we're mocking things pretty thoroughly.

This is the traceback I'm seeing in the test logs:

 2022-10-17 11:23:33,024 261 INFO odoo odoo.addons.letsencrypt.tests.test_letsencrypt: ====================================================================== 
2022-10-17 11:23:33,024 261 ERROR odoo odoo.addons.letsencrypt.tests.test_letsencrypt: ERROR: TestLetsencrypt.test_dns_challenge
Traceback (most recent call last):
  File "/opt/odoo-venv/lib/python3.8/site-packages/mock/mock.py", line 1346, in patched
    return func(*newargs, **newkeywargs)
  File "/__w/server-tools/server-tools/letsencrypt/tests/test_letsencrypt.py", line 114, in test_dns_challenge
    self.env["letsencrypt"]._cron()
  File "/__w/server-tools/server-tools/letsencrypt/models/letsencrypt.py", line 94, in _cron
    authzr = self._get_authorization_resource(client, main_domain, domains)
  File "/__w/server-tools/server-tools/letsencrypt/models/letsencrypt.py", line 265, in _get_authorization_resource
    return client.new_order(csr)
  File "/opt/odoo-venv/lib/python3.8/site-packages/acme/client.py", line 714, in new_order
    response = self._post(self.directory['newOrder'], order)
  File "/opt/odoo-venv/lib/python3.8/site-packages/acme/client.py", line 114, in _post
    return self.net.post(*args, **kwargs)
  File "/opt/odoo-venv/lib/python3.8/site-packages/acme/client.py", line 1289, in post
    return self._post_once(*args, **kwargs)
  File "/opt/odoo-venv/lib/python3.8/site-packages/acme/client.py", line 1303, in _post_once
    response = self._check_response(response, content_type=content_type)
  File "/opt/odoo-venv/lib/python3.8/site-packages/acme/client.py", line 1149, in _check_response
    raise messages.Error.from_json(jobj)
acme.messages.Error: urn:ietf:params:acme:error:rejectedIdentifier :: The server will not issue certificates for the identifier :: Error creating new order :: Cannot issue for "*.example.com": The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy

I couldn't see any obvious recent changes to the acme package, though I'll admit I don't really know what I'm looking for.

@thomaspaulb
Copy link
Contributor

@aisopuro The failing tests are from letsencrypt, have asked @NL66278 to try and fix them in a separate PR, then we can rebase this one when it is merged.

@thomaspaulb
Copy link
Contributor

Ah, you had already come to the same conclusion I see.

@NL66278
Copy link
Contributor

NL66278 commented Oct 19, 2022

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

Sorry @NL66278 you are not allowed to rebase.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@NL66278
Copy link
Contributor

NL66278 commented Oct 19, 2022

@aisopuro Please rebase your branch on latest 15.0. This now contains a fix by @StefanRijnhart that should solve the letsencrypt problems.

naglis and others added 20 commits October 20, 2022 09:53
* [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
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/
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.
OCA-git-bot and others added 12 commits October 20, 2022 09:53
Because post_load is called after odoo.service.server start
It has already registered the unpatched   odoo.service.wsgi_server.application
So patch it here too.
This enables wsgi performance reporting with sentry_traces_sample_rate
The test code uses a "mock" Transport object to ensure that events are
stored locally in memory, instead of triggering network requests.

The Sentry client is cleaned up once done, and this triggers a call to
capture_envelope, a different way of sending events to Sentry. Since
our mock class did not fully complete initialization, and also did
not provide an overriding method, the original was called, which
depends on proper initialization to work.

We introduce an override for capture_envelope: as it is meant to be
a "sibling" to capture_event, it makes sense for us to also make sure
events registrered in this way are intercepted, even if we don't
currently expect any of our tests to explicitly cause it to be used.
@aisopuro
Copy link
Contributor Author

Rebased and pushed, thanks for the help @NL66278

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@thomaspaulb
Copy link
Contributor

/ocabot merge nobump
/ocabot migration sentry

@OCA-git-bot OCA-git-bot added this to the 15.0 milestone Oct 29, 2022
@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 15.0-ocabot-merge-pr-2428-by-thomaspaulb-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot mentioned this pull request Oct 29, 2022
54 tasks
@OCA-git-bot OCA-git-bot merged commit 95b9975 into OCA:15.0 Oct 29, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at ca8af23. Thanks a lot for contributing to OCA. ❤️

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.