8000 Remove downloaded package archives after extracting them by kjagiello · Pull Request #11640 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

Remove downloaded package archives after extracting them #11640

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 9, 2024
Merged

Remove downloaded package archives after extracting them #11640

merged 1 commit into from
Oct 9, 2024

Conversation

kjagiello
Copy link
Contributor
@kjagiello kjagiello commented Oct 4, 2024 •
edited
Loading

Motivation

As mentioned in #11639, I have noticed that python -m localstack.cli.lpm install is leaving behind the downloaded package archives. This results in those files ending up in the published Docker image.

$ docker run --entrypoint bash -it localstack/localstack:3.8.0
root@40c78125f557:/opt/code/localstack# ls -lah /tmp/localstack
total 184M
drwxrwxrwx 1 root root  116 Oct  3 19:57 .
drwxrwxrwt 1 root root   50 Oct  3 19:57 ..
-rw-r--r-- 1 root root    0 Oct  3 17:38 .marker
-rw-r--r-- 1 root root 184M Oct  3 19:57 OpenJDK11U-jdk_aarch64_linux_hotspot_11.0.24_8.tar.gz
drwxr-xr-x 1 root root    0 Oct  3 19:57 state

Changes

This PR solves the issue by removing the archive after extracting a package. This should result in a 184 MB reduction of the localstack docker image.

Testing

  1. Build the image:
IMAGE_NAME="localstack/localstack" ./bin/docker-helper.sh build
  1. Make sure that there are no archives in /tmp/localstack:
docker run --entrypoint ls -it localstack/localstack:latest -lah /tmp/localstack

@kjagiello kjagiello requested a review from alexrashed as a code owner October 4, 2024 20:05
localstack-bot
localstack-bot reviewed Oct 4, 2024
View reviewed changes
Copy link
Collaborator
@localstack-bot localstack-bot left a comment

Choose a reason for hiding this comment

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

Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.

@localstack-bot
Copy link
Collaborator
localstack-bot commented Oct 4, 2024 •
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@kjagiello
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

localstack-bot added a commit that referenced this pull request Oct 4, 2024
@localstack-bot
@kjagiello has signed the CLA in #11640
2cf46dd
@kjagiello kjagiello marked this pull request as draft October 5, 2024 08:30
@kjagiello kjagiello changed the title Remove downloaded package archives after extracting them Remove downloaded package archives after installing them Oct 5, 2024
@kjagiello kjagiello force-pushed the remove-downloaded-archives branch from 8f12ade to 07c0d70 Compare October 5, 2024 09:15
@kjagiello kjagiello marked this pull request as ready for review October 5, 2024 12:37
alexrashed
alexrashed reviewed Oct 7, 2024
View reviewed changes
Copy link
Member
@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution, @kjagiello!
Your removal actually looks fine, but I think we should tackle the root cause here.
It seems like the new Java package installer (introduced with #11139) does not properly clean up after itself. /cc @viren-nadkarni
I think we should actually remove the archive here after the extraction of the newly downloaded archive here:

localstack/localstack-core/localstack/packages/core.py

Lines 108 to 128 in 958890a

def _install(self, target: InstallTarget) -> None:
target_directory = self._get_install_dir(target)
mkdir(target_directory)
download_url = self._get_download_url()
archive_name = os.path.basename(download_url)
download_and_extract(
download_url,
retries=3,
tmp_archive=os.path.join(config.dirs.tmp, archive_name),
target_dir=target_directory,
)
if self.extract_single_directory:
dir_contents = os.listdir(target_directory)
if len(dir_contents) != 1:
return
target_subdir = os.path.join(target_directory, dir_contents[0])
if not os.path.isdir(target_subdir):
return
os.rename(target_subdir, f"{target_directory}.backup")
rm_rf(target_directory)
os.rename(f"{target_directory}.backup", target_directory)

@viren-nadkarni, what do you think?

@viren-nadkarni
Copy link
Member

Great catch @kjagiello !

I agree with @alexrashed , this should happen in the packaging module right after extraction is complete. This will fix the issue not just for java but also for other packages.

@alexrashed alexrashed added the semver: patch Non-breaking changes which can be included in patch releases label Oct 7, 2024
@alexrashed
Copy link
Member

@kjagiello, do you think you could adjust your PR such that it fixes the ArchiveDownloadAndExtractInstaller instead of the removing the leftover in the Dockerfile? Otherwise I can take over if you want.

@kjagiello
Copy link
Contributor Author
kjagiello commented Oct 7, 2024 •
edited
Loading

Ah, that was actually the first approach I went to, then changed my mind, because I (wrongly) assumed it was intentional on your end. Should have stuck with my gut feeling 😄 I will adjust the PR shortly.

@kjagiello kjagiello changed the title Remove downloaded package archives after installing them Remove downloaded package archives after extracting them Oct 7, 2024
@kjagiello kjagiello force-pushed the remove-downloaded-archives branch from 07c0d70 to 8813bac Compare October 7, 2024 10:11
alexrashed
alexrashed approved these changes Oct 7, 2024
View reviewed changes
Copy link
Member
@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for jumping on the comment and adjusting the PR! The code is simple and clean. 💯
@viren-nadkarni Anything from your side that we should maybe keep in mind here?

viren-nadkarni
viren-nadkarni approved these changes Oct 8, 2024
View reviewed changes
Copy link
Member
@viren-nadkarni viren-nadkarni left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix 🙌

@alexrashed alexrashed merged commit 00a6ebf into localstack:master Oct 9, 2024
33 checks passed
@viren-nadkarni viren-nadkarni mentioned this pull request Oct 14, 2024
Closed
1 task
macnev2013 pushed a commit that referenced this pull request Oct 22, 2024
@kjagiello @macnev2013
Remove downloaded package archives after extracting them (#11640)
128107e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@localstack-bot localstack-bot localstack-bot left review comments

@alexrashed alexrashed alexrashed approved these changes

@viren-nadkarni viren-nadkarni viren-nadkarni approved these changes

Assignees
No one assigned
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kjagiello @localstack-bot @viren-nadkarni @alexrashed
0