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

Use proxy for caching Pypi dependencies #128

Merged
merged 7 commits into from
Jan 3, 2023

Conversation

amenasria
Copy link
Contributor

What does this PR do?

Implements usage of caching proxy for Pypi dependencies.

Same as #105 but for Python dependencies.

Motivation

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Reviewer's Checklist

@amenasria amenasria requested a review from a team as a code owner December 29, 2022 10:16
Copy link
Collaborator

@KSerrania KSerrania left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but we can't verify the omnibus build behavior in the test build done here until DataDog/datadog-agent#14473 is merged (because this repository uses main of datadog-agent to do its test runs). I suggest we merge the datadog-agent PR first, then redo a test build here, and check that the PIP_CONFIG_FILE env variable is properly set in the omnibus environment for the relevant commands.

@amenasria
Copy link
Contributor Author

amenasria commented Dec 30, 2022

Looks good to me, but we can't verify the omnibus build behavior in the test build done here until DataDog/datadog-agent#14473 is merged (because this repository uses main of datadog-agent to do its test runs). I suggest we merge the datadog-agent PR first, then redo a test build here, and check that the PIP_CONFIG_FILE env variable is properly set in the omnibus environment for the relevant commands.

Agreed ! Will wait for #14473 to be merged !

@amenasria
Copy link
Contributor Author

So it seems to be working (see here) !

Looking in indexes: ***datadog.jfrog.io/artifactory/api/pypi/agent-pypi/simple

The thing is though that for each requirement in https://raw.githubusercontent.com/DataDog/datadog-agent-buildimages/main/requirements.txt, the requirement is already satisfied because of the Cache brew deps step in the CI.

Since the Pypi caching is working for datadog-agent I think it's fine to say it's working fine !

@amenasria
Copy link
Contributor Author

I still have a question though, I looked into the other builds in datadog-agent (the ones in the package_build step of the CI), and I can't find the same pattern i.e a place where we using Github Actions cache.

I'm just wondering about the key field we're using here:

key: macos-11-build-cache-${{ hashFiles('./scripts/builder_setup.sh') }}

Does that mean that the cache won't get updated until the ./scripts/builder_setup.sh file is changed ? If this is true maybe I should change it in this PR so that the cache is updated with our Artifactory repository dependencies instead of the previous one ? Maybe it won't change anything, I'm just wondering aha !

@amenasria amenasria requested a review from KSerrania January 2, 2023 09:18
@KSerrania
Copy link
Collaborator

KSerrania commented Jan 2, 2023

Discussed offline, but for context:

Does that mean that the cache won't get updated until the ./scripts/builder_setup.sh file is changed ?

That is correct, as the goal is to only redo the builder setup when the setup script is modified (because that setup takes 30 minutes). One other way to force the cache to be rebuilt is to change some other part of the cache key, like:

key: macos-11-build-cache-${{ hashFiles('./scripts/builder_setup.sh') }}-v2

@amenasria amenasria merged commit c8a64a4 into master Jan 3, 2023
@amenasria amenasria deleted the amenasria/use-python-caching-proxy branch January 3, 2023 12:49
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.

2 participants