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

feature: Automatically pass SOURCE_DATE_EPOCH to Linux containers #1589

Merged
merged 1 commit into from Sep 18, 2023
Merged

feature: Automatically pass SOURCE_DATE_EPOCH to Linux containers #1589

merged 1 commit into from Sep 18, 2023

Conversation

dalcinl
Copy link
Contributor

@dalcinl dalcinl commented Aug 27, 2023

@dalcinl
Copy link
Contributor Author

dalcinl commented Aug 27, 2023

@joerick I listed you as a co-author of the commit, as the actual implementation follows your suggestion in #1584. Feel free to amend the commit with your digital signature and force push. Otherwise, I can remove the Co-authored-by line if you prefer.

@joerick
Copy link
Contributor

joerick commented Sep 6, 2023

Thank you @dalcinl! A test for this somewhere would be good - probably an integration test would be appropriate to ensure we never lose this. Let me know if you need any help/pointers!

@dalcinl
Copy link
Contributor Author

dalcinl commented Sep 6, 2023

@joerick Done, but I got fed up fighting pre-commit and its code uglification. Maybe I should do it the following way, but I'll not keep wasting my time without your final word on it.

env_manager = modified_os_environ(CIBUILDWHEEL="1", SOURCE_DATE_EPOCH="42")
oci_manager = OCIContainer(engine=container_engine, image=DEFAULT_IMAGE)
with env_manager, oci_manager:
   ...

PS: As you did not have a test for CIBUILDWHEEL being passed to containers, I took the liberty of adding a test for it.

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

Thanks!

PS: As you did not have a test for CIBUILDWHEEL being passed to containers, I took the liberty of adding a test for it.

There is an integration test for this in /test, but good to have it here too.

unit_test/oci_container_test.py Outdated Show resolved Hide resolved
@dalcinl
Copy link
Contributor Author

dalcinl commented Sep 7, 2023

There is an integration test for this in /test, but good to have it here too.

As I'm not a pytest user, it was not clear to me if these tests were actually exercising the passing through the docker/podman wall.

At this point, I believe I've addressed all your comments. Please note that I made a relatively small test change since your last review: rather than modifying an existing test function, now I'm adding a new one to exercise environment passing.

Co-authored-by: Joe Rickerby <joerick@mac.com>
@dalcinl
Copy link
Contributor Author

dalcinl commented Sep 18, 2023

@joerick I believe this PR is ready for merging. There are CI workflows waiting for approval. The Cirrus CI failure is most likely unrelated and may need a manual restart.

Copy link
Contributor

@joerick joerick 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! Thank you

@joerick joerick merged commit 80a54b0 into pypa:main Sep 18, 2023
22 of 23 checks passed
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.

None yet

2 participants