This repository was archived by the owner on Oct 1, 2024. It is now read-only.
Rewrite chroot_setup in Python - #79
Open
markirish wants to merge 31 commits into
Open
Conversation
Initial rough attempt at converting chroot_setup.sh to chroot_setup.py Currently has a bugs, may not run, but every function save one has been converted to rough Python Signed-off-by: Mark Irish <mirish@ibm.com>
Author
|
Created draft pull request so @abmusse , @kadler , and myself have a place to discuss the code. Code hasn't be intensively tested, I'm a bit of a coward with firing it off with all the |
Author
|
One thing I didn't know: In the |
abmusse
reviewed
Dec 1, 2020
abmusse
reviewed
Dec 1, 2020
abmusse
reviewed
Dec 1, 2020
abmusse
reviewed
Dec 1, 2020
kadler
reviewed
Dec 8, 2020
kadler
left a comment
kadler
left a comment
Member
There was a problem hiding this comment.
Preliminary comments. This seems like a straight-forward port from the original shell script. While that definitely has improvements over the shell script, I'd like to go even further.
| # TODO: documentation | ||
| def initial_check (): | ||
| if (os.path.exists('/QOpenSys/usr/bin')): | ||
| CHROOT_DEBUG = False |
Member
There was a problem hiding this comment.
By Python conventions this would be a constant, but it's being altered below. Instead, we can drop this variable and reference args.verbose instead.
In addition, @abmusse mentioned using args.verbose to adjust the logging level and then just calling the appropriate logging function and we never have to even check the variable anywhere else.
Member
There was a problem hiding this comment.
I guess this is separate from verbosity of logging as when this is set, no action will be performed but it will still go through showing what would be done. Many utilities have command line options for this, usually with --dry-run or -n. I think that would make more sense to do something like that.
Member
There was a problem hiding this comment.
To me the CHROOT_DEBUG variable was a misnomer because it really was used to guard against running commands on non IBM i systems as it was only set to true in the else leg of initial_check when the script was run on non IBM i system then the script exited.
Therefore none of the actions would run anyway so why did the script even bother to include checks for CHROOT_DEBUG in the actions? See
Line 232 in 494fe28
Every chroot_* action checked if debug was == 0 (passed the is IBM i check) , To prevent running any action on non IBM i system.
See
Line 24 in 494fe28
Member
There was a problem hiding this comment.
I think we can get rid of all the if not CHROOT_DEBUG in the chroot_* actions. We just need to exit the script properly in the initial_check if we are not running on IBM i.
Author
Fix all build and runtime errors Ensure that basic functionality works Signed-off-by: Mark Irish <mirish@ibm.com>
Author
|
Ok I changed the name and apparently it didn't like that for keeping track of everything. What I changed:
I have run this with DEBUG = True, so that it tells me what it will run but doesn't actually run it. I think I will try to run these functions/actions piecemeal, as I'm afraid of them doing something bad like mucking with the entire file structure. I suppose I could try to run it inside of a chroot (to create a sub chroot?), and that way I wouldn't have to be afraid... |
Change all print statements to their Python functions Add documentation Implement chroot_ln_rel function Fix bugs Add help text
Author
|
Ok! This is finally to a stage where it is working and I feel confident that it can do what it needs to do. I built with a The only thing now is to clean it up, change wording on things like help text (or that goofy ASCII text art), and refactor where desired. |
markirish
marked this pull request as ready for review
December 16, 2020 03:19
Signed-off-by: Mark Irish <mirish@ibm.com>
abmusse
reviewed
Dec 22, 2020
Comment on lines
+367
to
+368
| mylist.append(SCRIPT_DIR + "/chroot_includes.lst") | ||
| mylist.append(SCRIPT_DIR + "/chroot_minimal.lst") |
Member
There was a problem hiding this comment.
After #75, we should now use default.lst instead.
Suggested change
| mylist.append(SCRIPT_DIR + "/chroot_includes.lst") | |
| mylist.append(SCRIPT_DIR + "/chroot_minimal.lst") | |
| mylist.append(f'{SCRIPT_DIR}/default.lst') |
Member
There was a problem hiding this comment.
This branch would need to be rebased with master to pull in config/default.lst
abmusse
reviewed
Dec 22, 2020
| # Only chroot_directory was provided to the script: | ||
| # Ask if minimal with includes chroot is desired | ||
| if not auto_yes: | ||
| val = input("Would you like to create a minimal chroot w/ includes @ " + CHROOT_DIR + "? [y/N]:") |
Member
There was a problem hiding this comment.
Suggested change
| val = input("Would you like to create a minimal chroot w/ includes @ " + CHROOT_DIR + "? [y/N]:") | |
| val = input("Would you like to create a default chroot environment @ " + CHROOT_DIR + "? [y/N]:") |
Member
There was a problem hiding this comment.
Will wait for this branch to get rebased with master for this change and #79 (comment)
kadler
reviewed
Dec 22, 2020
| /QOpenSys/usr/lib/libpthread.a /QOpenSys/usr/ccs/lib/libpthread.a | ||
| /QOpenSys/usr/lib/libpthreads.a /QOpenSys/usr/ccs/lib/libpthreads.a | ||
| /QOpenSys/usr/lib/libpthreads_compat.a /QOpenSys/usr/ccs/lib/libpthreads_compat.a | ||
| /QOpenSys/usr/lib/libpthreads_compat.a /QOpenSys/usr/ccs/lib/bpthreads_compat.a |
Comment on lines
+614
to
+620
| # (system "CHGAUT OBJ('/QOpenSys/ranger/home/ranger') USER(RANGER) DTAAUTRWX) # OBJAUT(*ALL) SUBTREE(*ALL)") | ||
| # -- variable substitution example (mydir and myuser) -- | ||
| # > ./chroot_setup.sh chroot_system.lst /QOpenSys/ranger mydir=/QOpenSys/ranger myuser=RANGER | ||
| # > ./chroot_setup.sh chroot_system.lst /QOpenSys/ranger mydir=/QOpenSys/nger # myuser=RANGER | ||
| # | ||
| :system | ||
| # CHGAUT OBJ('/QOpenSys/ranger/home/ranger') USER(RANGER) DTAAUT(*RWX) OBJAUT(*ALL) SUBTREE(*ALL) | ||
| # CHGAUT OBJ('mydir/home/myuser') USER(myuser) DTAAUT(*RWX) OBJAUT(*ALL) SUBTREE(*ALL) | ||
|
|
||
|
|
||
| # CHGAUT OBJ('/QOpenSys/ranger/home/ranger') USER(RANGER) DTAAUT(*RWX) JAUT# (*ALL) SUBTREE(*ALL) | ||
| # CHGAUT OBJ('mydir/home/myuser') USER(myuser) DTAAUT(*RWX) OBJAUT(*ALL) SUBTREE# (*ALL) |
kadler
reviewed
Dec 22, 2020
kadler
reviewed
Dec 22, 2020
kadler
reviewed
Dec 22, 2020
kadler
left a comment
kadler
left a comment
Member
There was a problem hiding this comment.
In addition to the specific comments, I'd also like to add that the code should be run through pylint - for sure the indentation should be adjusted to 4 spaces.
Comment on lines
+385
to
+391
| # check that the .lst file exists | ||
| if not os.path.isfile(CHROOT_LIST): | ||
| print(f"{CHROOT_LIST} cannot be found in {os.cwd}") | ||
| sys.exit(-7) | ||
| else: | ||
| # append file name to end of list | ||
| mylist.append(os.readlink(CHROOT_LIST)) |
Member
There was a problem hiding this comment.
This is a TOCTOU race condition. Instead, call os.readlink unconditionally and handle the FileNotFoundException.
Suggested change
| # check that the .lst file exists | |
| if not os.path.isfile(CHROOT_LIST): | |
| print(f"{CHROOT_LIST} cannot be found in {os.cwd}") | |
| sys.exit(-7) | |
| else: | |
| # append file name to end of list | |
| mylist.append(os.readlink(CHROOT_LIST)) | |
| try: | |
| mylist.append(os.readlink(CHROOT_LIST)) | |
| except FileNotFoundException: | |
| print(f"{CHROOT_LIST} cannot be found in {os.cwd}") | |
| sys.exit(-7) |
Member
There was a problem hiding this comment.
Probably should open them too, while we're at it. If we still want to print the file path later, we could store a tuple(path, file obj) in the array.
Member
There was a problem hiding this comment.
Fixed up in d80220a
Instead of checking if the file exists I just append the file name to the last. later in the chroot_setup function the file is opened for reading.
Comment on lines
+409
to
+415
| for key in globals_dictionary: | ||
| os.environ[key] = globals_dictionary[key] | ||
|
|
||
| chroot_setup(chroot_file) | ||
|
|
||
| for key in globals_dictionary: | ||
| del os.environ[key] |
Member
There was a problem hiding this comment.
Is there a reason we add and remove all the globals for each file?
Member
There was a problem hiding this comment.
Personally, I think we should stop using environment variables for this behavior and use a dictionary for the passed mappings instead. Of course, this does break any scripts that relied on environment variables which were set prior to invoking the script. If such list files exist, we could add a fallback to check the environ if it's not found in the mapping.
Member
There was a problem hiding this comment.
Actually these globals were never used as environment variables in the original script. These are named variables passed into chroot_setup.sh that get substitute placeholder values in the xxx.lst files. See https://github.com/IBM/ibmichroot#advanced.
Comment on lines
+244
to
+268
| elif line.startswith(':'): | ||
| action = action_dictionary[line] | ||
|
|
||
| # Execute action | ||
| else: | ||
| if action is not None: | ||
| action(line) | ||
|
|
||
| # Mapping the .lst action keys to the functions that run on their values | ||
| action_dictionary = { | ||
| ':file' : chroot_setup, | ||
| ':mkdir' : chroot_mkdir, # done | ||
| ':ln_fix_rel' : chroot_ln_fix_rel, # done | ||
| ':ln_rel' : chroot_ln_rel, # done | ||
| ':ln' : chroot_ln, # done | ||
| ':mknod' : chroot_mknod, # done | ||
| ':cp_dir' : chroot_cp_dir, # done | ||
| ':cp' : chroot_cp, # done | ||
| ':chmod_dir' : chroot_chmod_dir, # done | ||
| ':chmod' : chroot_chmod, # done | ||
| ':chown_dir' : chroot_chown_dir, # done | ||
| ':chown' : chroot_chown, # done | ||
| ':system' : chroot_system, # done | ||
| ':sh' : chroot_sh # done | ||
| } |
Member
There was a problem hiding this comment.
If we wanted to enforce a consistent function naming scheme, this mapping could go away:
elif line.startswith(':'):
function = f"chroot_{line[1:]}"
action = globals()[function]In addition, it might be good to catch the KeyError if the user gives an unexpected action.
Member
There was a problem hiding this comment.
The only inconsistent action -> function name is:
':file' : chroot_setup,
We could make a wrapper chroot_file function that calls chroot_setup
abmusse
reviewed
Jan 27, 2021
abmusse
reviewed
Jan 27, 2021
abmusse
reviewed
Jan 27, 2021
abmusse
reviewed
Jan 27, 2021
abmusse
reviewed
Feb 3, 2021
abmusse
reviewed
Feb 6, 2021
- add input_yes function - Use * for chroot_type nargs
abmusse
reviewed
Feb 11, 2021
abmusse
reviewed
Feb 11, 2021
abmusse
reviewed
Feb 11, 2021
abmusse
reviewed
Feb 11, 2021
Member
|
@kadler I've fixed up the requested changes and added flake8 linter action derived from default pythonpackage.yml After fixing up initial linter issues Note these linter issues are considered warnings. ...
run: |
# stop the build if there are Python syntax errors or undefined names
flake8 chroot_setup --count --select=E9,F63,F7,F82 --show-source --statistics
# exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide
flake8 chroot_setup --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics
... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Initial rough attempt at converting chroot_setup.sh to chroot_setup.py
Currently has a bugs, may not run, but every function save one has been converted to rough Python
Signed-off-by: Mark Irish mirish@ibm.com