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

Add docker rootless feature flag and its implementation for supporting docke rootless environment #818

Merged
merged 1 commit into from
Feb 25, 2024

Conversation

kimsehwan96
Copy link
Contributor

resolve: #817

Add feature flag dockerRootless and default value false.

Change lib/pip.js file permission change logic for supporting docker rootless environment.

I think that its better add some document about this in README.md

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Hey @kimsehwan96 👋 Thanks a lot for your contribution - do you think it would be possible to cover this by test by any chance, or it's gonna be rather hard to include in the testing suite?

@kimsehwan96
Copy link
Contributor Author

Hello @pgrzesik . I think that this change should be tests in CI(Github action) level. Becasue it doesn't work that we expected when enable that feature flag in Not rootless docker environment.

I will find how to test it in unit test or CI level test in github action. I think that it will not very hard to include this in the testing suite!

@pgrzesik
Copy link
Contributor

Hey @kimsehwan96 👋 Could you fix the formatting as listed in the failed CI build? I think having automated tests for this would be tricky, but let's make sure it's not breaking anything else in the process 👍

@kimsehwan96
Copy link
Contributor Author

kimsehwan96 commented Feb 10, 2024

Hello @pgrzesik . Sorry for late works.. As you mentioned, apply automated tests for this change is tricky. I underestimated about it 🥲. And I failed to devote much of time.

By the way, I will applying the formatting and inform about it to you.

When I have some free time from the coming week, I will append some comment about which cases of environment this will be needed with some detailed information.

Thank you !

@kimsehwan96
Copy link
Contributor Author

I applied formatting for this change ! @pgrzesik

I want to merge 2 commits which are for applying dockerRootless option and prettier . So I rebased and squashed 2 commits into 1 and force pushed to this branch. Is it okay?

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Thank you @kimsehwan96 🙇

@pgrzesik pgrzesik merged commit fa9ac03 into serverless:master Feb 25, 2024
2 checks passed
@kimsehwan96 kimsehwan96 deleted the add-docker-rootless-support branch February 29, 2024 02:12
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.

Add docker rootless feature flag for using this plugin in docker rootless environment
2 participants