-
Notifications
You must be signed in to change notification settings - Fork 100
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
Update PyPy in CI to 3.10 #1065
Conversation
🦙 MegaLinter status: ❌ ERROR
See detailed report in MegaLinter reports |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1065 +/- ##
==========================================
- Coverage 81.21% 75.01% -6.21%
==========================================
Files 190 190
Lines 19747 19747
Branches 3468 3468
==========================================
- Hits 16038 14813 -1225
- Misses 2720 3911 +1191
- Partials 989 1023 +34 ☔ View full report in Codecov by Sentry. |
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.
A couple minor points of feedback. Overall the switch from pypy3.8 to pypy3.10 looks good.
.github/containers/Dockerfile
Outdated
@@ -96,7 +96,7 @@ RUN echo 'eval "$(pyenv init -)"' >>${HOME}/.bashrc && \ | |||
pyenv update | |||
|
|||
# Install Python | |||
ARG PYTHON_VERSIONS="3.11 3.10 3.9 3.8 3.7 3.12 2.7 pypy2.7-7.3.12 pypy3.8-7.3.11" | |||
ARG PYTHON_VERSIONS="3.11 3.10 3.9 3.8 3.7 3.12 2.7 pypy2.7-7.3.12 pypy3.10-7.3.15 pypy3.8-7.3.11" |
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.
I think we can just drop pypy3.8 here now can't we?
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.
I think there might be at least 1 framework that doesn't support pypy3.8, I have 3 left that haven't passed yet with 3.10. Will take another look tomorrow to see if I can get them all to pass and I'll drop 3.8 if that works.
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.
I think I got them all running now, I dropped 3.8 completely
@@ -118,7 +118,7 @@ def reset(wrapped, instance, args, kwargs): | |||
return reset | |||
|
|||
|
|||
@reset_agent_config(INI_FILE_WITHOUT_UTIL_CONF, ENV_WITHOUT_UTIL_CONF) | |||
@reset_agent_config(INI_FILE_WITHOUT_UTIL_CONF, {"NEW_RELIC_HOST": "collector.newrelic.com"}) |
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.
It's concerning to me that the HOST is needed here. It shouldn't need the host to default the OTLP_HOST. Can you explain more about this change/why this test was failing? I just tried running it locally and it didn't fail for me.
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 test is really annoying and fails if you've got NEW_RELIC_HOST set to point to staging. It causes my local instance to fail all the time.
If you've got a better idea to make this test not fail I'd be happy to change it.
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.
I was thinking we had other tests for when this was specifically set that would make this change to it a duplicate test but I don't actually see any so I guess this change is ok.
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.
Looks good!
@@ -118,7 +118,7 @@ def reset(wrapped, instance, args, kwargs): | |||
return reset | |||
|
|||
|
|||
@reset_agent_config(INI_FILE_WITHOUT_UTIL_CONF, ENV_WITHOUT_UTIL_CONF) | |||
@reset_agent_config(INI_FILE_WITHOUT_UTIL_CONF, {"NEW_RELIC_HOST": "collector.newrelic.com"}) |
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.
I was thinking we had other tests for when this was specifically set that would make this change to it a duplicate test but I don't actually see any so I guess this change is ok.
Last test run passing, switching the CI image tag back to latest https://github.com/newrelic/newrelic-python-agent/actions/runs/7982302622?pr=1065 |
* Error trace linking (#1019) * Add guid to error event & trace intrinsics w/o DT * Update tests to show guid in intrinsics sans DT * Fix cross agent DT tests and some pylint errors * Remove memcached testing for Python 2.7 * Temporarily pin hypercorn to <0.16 * Remove guid from TracedError & add to intrinsics * Rearrange intrinsics to remove redundancy * Move guid to make it agnostic to DT * Change param initialization type * Use getattr instead of calling root.guid directly * Revert cross agent tests and add guid to DT as well * Separate outside transaction error trace linking * Remove guid from DT specific attribute checks * Fix slow SQL test * Remove guid from JSON * Revert error_trace_collector validator --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * Update PyPy in CI to 3.10 (#1065) * Update PyPy3.8 to PyPy3.10 in CI * Fix broken utilization test * Pin CI sha to dev image * Fix failing sklearn test * Unroll sklearn changes * Update falcon tests for 2024 SLAs * Remove pypy38 from image * Unpin dev CI image sha * Change all pypy38 to pypy310 --------- Co-authored-by: Lalleh Rafeei <84813886+lrafeei@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Overview