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

Fix, use "service.build.ulimits" when it is present instead of "service.ulimits" #906

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

camspiers
Copy link
Contributor

@camspiers camspiers force-pushed the fixes/ulimit-build-args branch from ed43705 to 54561f0 Compare April 8, 2024 07:27
Copy link
Collaborator

@p12tic p12tic 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 in principle, but needs unit tests.

@camspiers camspiers force-pushed the fixes/ulimit-build-args branch 3 times, most recently from b8c3a97 to e2bb65b Compare April 9, 2024 02:41
@camspiers
Copy link
Contributor Author

Hey @p12tic thanks for reviewing the PR. I have updated with unit tests, but I am not 100% on what I have done.

At the moment I am testing both that the ulimits are passed correctly to the build command, and that the ulimits are applied during the build. However I could solely test that the right arguments are passed to the build command using the dry-run flag and remove the actual execution of the ulimit command in the build.

@camspiers camspiers requested a review from p12tic April 9, 2024 02:49
@p12tic p12tic force-pushed the fixes/ulimit-build-args branch from e2bb65b to 157bcb8 Compare April 17, 2024 16:42
@p12tic p12tic force-pushed the fixes/ulimit-build-args branch from 157bcb8 to cdcedeb Compare April 17, 2024 16:46
@p12tic
Copy link
Collaborator

p12tic commented Apr 17, 2024

Rebased and shortened commit message to max 72 chars allowed by git.

@p12tic p12tic merged commit 1da2f85 into containers:main Apr 17, 2024
8 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.

2 participants