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

Added pybamm user id to 1000 and set its group to root #3947

Merged
merged 5 commits into from
Apr 3, 2024

Conversation

santacodes
Copy link
Contributor

@santacodes santacodes commented Mar 30, 2024

Description

Changed the pybamm userID to 1000 and groupID to 0 (root).

Related to #3312
docker

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@santacodes
Copy link
Contributor Author

By default the default user pybamm inside the docker container doesn't have root permissions which would restrict the user from using fake root environment or sudo operations to install additional packages and also to use commands like chmod which might be useful to run executables. To solve this pybamm user is added to the root group (0).

@agriyakhetarpal
Copy link
Member

Thanks for this @santacodes – could you spin up a container and verify your changes if you haven't done so already? The expected behaviour is that the pybamm user should be able to install packages as you mentioned, so installing something via apt-get should work via the entry point for the container – if that does, we should be good to go. Another thing you could test out is whether you can copy things to and from the location pointed to by LD_LIBRARY_PATH.

@santacodes
Copy link
Contributor Author

@agriyakhetarpal Sure I gave it a try and these are the points to be noted -

  • The docker container doesn't have the sudo command by default due to security issues as typically they are used for microservices. So to overcome this apt-get install sudo is added to dockerfile and sudo is installed at build-time. This shouldn't be a problem as this container is aimed for development purposes and not for deploying services.
  • I have added the user pybamm to the sudoers group which would enable it to use the sudo command.
  • The default password for the user pybamm is pybamm which is assigned at build-time. We can change it to something else and mention it in docs or something.
  • By entry point, I am guessing bash, as for a new container the apt-get packages are not synchronized so to ensure that sudo apt-get update needs to be issued before using any apt-get install command.
    aptdocker
  • Also for the LD_LIBRARY_PATH which is directed at /home/pybamm/.local/lib directory the cp command for copying files works.
    copyperms

Copy link
Member

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

Thanks for this @santacodes, can you also add some related useful documentation for this on the Install from Source (Docker) page?

@santacodes
Copy link
Contributor Author

Thanks for this @santacodes, can you also add some related useful documentation for this on the Install from Source (Docker) page?

Sure I will add them 😃

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.58%. Comparing base (78b7d25) to head (d51c420).
Report is 1 commits behind head on develop.

❗ Current head d51c420 differs from pull request most recent head e2b0715. Consider uploading reports for the commit e2b0715 to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3947   +/-   ##
========================================
  Coverage    99.58%   99.58%           
========================================
  Files          257      257           
  Lines        21196    21196           
========================================
  Hits         21108    21108           
  Misses          88       88           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@agriyakhetarpal agriyakhetarpal requested a review from arjxn-py April 2, 2024 18:14
Copy link
Member

@arjxn-py arjxn-py 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, thanks @santacodes

@arjxn-py arjxn-py merged commit 88658cf into pybamm-team:develop Apr 3, 2024
44 checks passed
js1tr3 pushed a commit to js1tr3/PyBaMM that referenced this pull request Aug 12, 2024
)

* added pybamm user id to 1000 and set the group to root

* added sudo and appended pybamm to sudoers

* added docs for adding pybamm user to root and sudoers group

* Update docs/source/user_guide/installation/install-from-docker.rst

Co-authored-by: Agriya Khetarpal <[email protected]>

---------

Co-authored-by: Agriya Khetarpal <[email protected]>
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.

3 participants