-
Notifications
You must be signed in to change notification settings - Fork 45
Release with [email protected] #565
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
Conversation
Dockerfile
Outdated
@@ -50,7 +50,7 @@ RUN PYTHONNODEBUGRANGES=1 python -OO -m compileall -b ./python/lib/$runtime/site | |||
# remove all .py files except ddtrace/contrib/*/__init__.py which are necessary | |||
# for ddtrace.patch to discover instrumationation packages. | |||
RUN find ./python/lib/$runtime/site-packages -name \*.py | grep -v ddtrace/contrib | xargs rm -rf | |||
RUN find ./python/lib/$runtime/site-packages/ddtrace/contrib -name \*.py | grep -v __init__ | xargs rm -rf | |||
# RUN find ./python/lib/$runtime/site-packages/ddtrace/contrib -name \*.py | grep -v __init__ | xargs rm -rf |
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.
Commenting out this line will have the impact of increasing the layer size significantly. We shouldn't do this.
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 will also have the impact of increasing cold start time.
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'm aware of the related drawbacks. Just want to first commenting it out to see it's related as the integration tests passed.
The specific error complained about logging
module. So maybe we just keep the logging
's py files?
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.
Updated.
Dockerfile
Outdated
RUN find ./python/lib/$runtime/site-packages/ddtrace/contrib -name \*.py | grep -v __init__ | xargs rm -rf | ||
RUN find ./python/lib/$runtime/site-packages/ddtrace/contrib -name "*.py" \ | ||
-not -path "./python/lib/$runtime/site-packages/ddtrace/contrib/internal/logging/*" \ | ||
| grep -v __init__ | xargs rm -rf |
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.
Unfortunately, this isn't the right fix. To better understand the problem, take a look at this line change in this PR: https://github.com/DataDog/dd-trace-py/pull/12153/files#diff-6a70144455f9fea41df44b3e578cfaa81ae9c5297e41cce9ef02a5b719013817R265
pyproject.toml
Outdated
@@ -28,7 +28,7 @@ classifiers = [ | |||
python = ">=3.8.0,<4" | |||
datadog = ">=0.51.0,<1.0.0" | |||
wrapt = "^1.11.2" | |||
ddtrace = ">=2.20.0" | |||
ddtrace = "==2.21.3" |
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.
Hmm, one concern here is that this change will also impact customers not using the layer.
I'm wondering if instead of changing the compatibility requirements here, we should just force an install of the specific ddtrace version in the Dockerfile.
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.
So basically, here
datadog-lambda-python/Dockerfile
Line 17 in b64be3b
RUN pip install . -t ./python/lib/$runtime/site-packages |
RUN pip install . ddtrace==2.21.3 -t ./python/lib/$runtime/site-packages
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.
Actually, I'm fine with however you wanna handle this @joeyzhao2018 since we're just gonna revert it back right away afterwards.
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.
The docker file is a better place i agree.
0316755
to
3b35b0b
Compare
@@ -14,7 +14,7 @@ ENV PATH=/root/.cargo/bin:$PATH | |||
|
|||
# Install datadog_lambda and dependencies from local | |||
COPY . . | |||
RUN pip install . -t ./python/lib/$runtime/site-packages | |||
RUN pip install . ddtrace==2.21.3 -t ./python/lib/$runtime/site-packages |
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.
⚪ Code Quality Violation
RUN pip install . ddtrace==2.21.3 -t ./python/lib/$runtime/site-packages | |
RUN pip install --no-cache-dir . ddtrace==2.21.3 -t ./python/lib/$runtime/site-packages |
use --no-cache-dir to avoid caching (...read more)
This rule states that Dockerfiles should not use a cache when installing packages. When building Docker images, Docker has a built-in caching mechanism that reuses instructions from previous builds, which can speed up the build process. However, when installing packages, this can lead to outdated packages being used, which might have security vulnerabilities or bugs.
It is important to avoid using a cache when installing packages because it ensures that the latest version of a package is always used. This reduces the risk of security vulnerabilities and bugs, and ensures that your application has the most up-to-date and secure dependencies.
When installing packages with pip in a Dockerfile, use the --no-cache-dir
option. This tells pip not to use a cache when installing packages, which ensures that the latest version of the package is always used. For example, instead of writing RUN pip install django
, write RUN pip install --no-cache-dir django
.
What does this PR do?
Release with [email protected]
Motivation
Include a fix for payload tagging for dynamodb
Testing Guidelines
Additional Notes
Types of Changes
Check all that apply