# Tag Info

13

Firstly, your installfromdownload function should be install_from_download (and have a docstring to go with it explaining what it does). This line: targetfolder = r"C:\Program Files (x86)\Mod Manager\Mods\f_" + filename[:-4] should probably check and make sure that the file actually ends with .zip (or whatever format you're actually expecting it to come ...

9

Your code and overall logic look good. In general the areas of improvement I see are There are some things you are inconsistent about, e.g., function parameters You are sending notification information down the pipeline. Some of the parameter declaration could be improved. I see that your are trying to make the code fit within a certain amount of ...

8

In addition to Yuushi's answer, I notice is that you're using os.system for copying files. This is insecure and unnecessary. If all you need to do is copy a file, use shutil.copy. Translated, a bit of your code might look like this: # assuming shutil is imported shutil.copy(r"C:\Program Files (x86)\Mod Manager\Mods\z_" + filename + ".scd", r"C:\...

8

Pretty nice script, I have mostly minor suggestions. Always double-quote variables used in command arguments The parameters in these command should be double-quoted to protect from word splitting and globbing: rsync -ah --progress $firefox_path /opt/$firefox_file ... rm $firefox_link ... tar -jxvf$firefox_file ... rm $firefox_file Avoid changing the ... 6 Your main function is hard to follow, but it seems to repeat its logic with subfolders. Generally something like that should be implemented as a recursive function. In pseudo-code, the basic structure of the recursive function would be: def copy_scds(scd, target_folder): unzip scd to target_folder if target_folder has scds: for each ... 6 2018/04/24 edit I missed an obvious bug: is_linux_mint is not implemented correctly. The results of a return on an empty output of a command substitution, with or without quotes, is implementation-defined: The value of the special parameter '?' shall be set to n, an unsigned decimal integer, or to the exit status of the last command executed if n is not ... 5 The output of your script is lovely, but it strikes me as a work in progress. There's nothing wrong with what is there, but it seems a few major pieces are missing. error checking What happens if one of the steps fails? As it stands the script will diligently proceed to the next step. If you actually checked the return codes from your apt commands you ... 5 Should this be a shell script? You mentioned that you wrote this script to push the packages to over 60 servers. If you have that many servers, then you should have a configuration management tool to do these kinds of tasks for you and much, much more. Typical solutions include Ansible, CFengine, Chef, and Puppet. Instead of copying the RPMs to each ... 5 Working with the ecosystem It important to know when not to write code. If you're going to work in the APT ecosystem, then you should work with other package management tools, rather than invent your own approach. There are at least two tools you should be leveraging: git-buildpackage is a suite of commands to create Debian packages based on Git ... 5 So, this might not be quite in line with your 100% automation approach, but the best way to provide a shell script access to a git repository is to use ssh keys to allow access to your git repository. Of course, this means you either need to log on and set up that key and grant access it to Git (goodbye 100% automation) or securely copy a private key that ... 4 Do you really have a good reason for this permission setup? RUN chmod 777 /root/startup.sh It's a bit ironic to use the -v flag of tar, and then redirecting the output to /dev/null here: RUN tar -xvzf httpd-2.4.18.tar.gz >/dev/null You can drop that flag and the redirection, the outcome should be the same. I'm not very familiar with Docker, but I'm ... 4 I see a number of things that may help you improve this program. I don't have a Windows machine handy, so these will all be Linux-based observations. For local installation, don't use sudo Right now, when the program is created for a local user, the use of sudo to copy it means it ends up owned by root. That means that although it's in the local user's ... 4 Possible Problems Don't use exact paths. I do not have a user called 'duncan', nor do I wish to. Instead use ~/Documents/fakeroot. However, I would argue that this is the point of /tmp, as you should want to purge the files from the system afterwards. My Arch install doesn't come with SQL preinstalled, so I highly doubt LFS would (Sure you can install it ... 4 If you are using bash then rather than mixing [ and [[, you should stick with one or the other, preferrably [[. See What is the difference between test, [ and [[ ? You might also want to consider preferring printf over echo for a more robust approach. See this amazingly detailed analysis of why printf is better than echo. 4 You've got 0 validation in there, so the slightest hiccup will halt your script halfway. That's fine for you, but for a user, there will be a script that runs, crashes along the way, and then leaves all sorts of junk lying about. You also have a "warning" for an unknown OS version, but then the os version seems to be required - without it, mppkg won't make ... 4 !!! Use this at your own risk. Fiddling with user's packages is scary business !!! You can monkey-patch the builtin import statement and upon encountering ModuleNotFoundError install whatever dependency is missing. Here is a small example for a script.py. You can of course extend this to feature multiple files or dynamically use the file given as input. ... 4 Some notes: This script is really very brittle. It'll break if any of the tools change in any significant way, if your disk layout changes (see below), or if the sequence of steps for whatever reason is not right in future versions of Arch Linux. It's a nice exercise to make sure you've understood the installation process, but in my experience these scripts ... 4 Make the process approach atomic The script first wipes out${war}/${domain}, then populates it with content from wget commands, and restores content from backup. The wget commands are a serious risk here. If any of the source sites is down or experiencing instability at the time the script runs, your site can end up in an inconsistent state. At the ... 3 A few comments: I’m not sure why you’re so keen to suppress any and all output from your RUN commands. That information can be useful when you’re trying to work out why your image hasn’t built properly. It can be a bit verbose when it goes fine, but it’s useful when it fails. (Docker’s inbuilt networking is sometimes a bit flakey for me, and apt-get and ... 3 A few small comments: The error message looks wrong to me. This is what the logic looks like to me: if (third party package is installed): install(in-house package) else: install(in-house package) install(in-house variant of third-party package) but the printed warning tells me that the in-house package isn’t installed. Shouldn’t it tell me ... 3 I'm not quite sure if your script should consist of elifs instead of ifs. I'm going to assume that it should be elifs since you don't want to be overwriting your links. If not just remove break or reverse the order of the directories. for dir in /usr/lib{/x86_64-linux-gnu,/i386-linux-gnu,64}; do if [[ -d$dir ]]; then sudo ln -s $dir/{... 3 The biggest problem I see with this code is the extreme level at which you intermingle your browser output with your PHP logic. Developing in this way is going to lead to a codebase that is nearly impossible to maintain. The basic structure of even teh simplest PHP page should be: <PHP logic> <View Output> Where <View Output> might have ... 3 (I know, reviving an OLD post, but the info I am sharing should be helpful to anyone new to Ansible that ends up here.) You're playbook looks pretty good, well done! There are a few minor things that would help make your Ansible fu better. First, your use of remote_user and specifically indicating that user as "root" makes your use of become_user ... 3 Wall of code The script looks like a wall of code. It would be a lot more pleasant to read if related groups of commands were separated from each other by blank lines, for example: # install steam wget https://steamcdn-a.akamaihd.net/client/installer/steam.deb sudo dpkg -i steam.deb sudo apt-get install -f # install atom wget https://atom.io/download/deb ... 3 The outer heredoc construct isn't really required. You do not verify that you have command line arguments. You do not assign your command line arguments to the variable domain. You use the variable domain when it is uninitialised. You make a number of assumptions, any one of which could bite you hard. You assume that the nginx package has been installed ... 3 Before relying on old VBS scripts, it pays to check what native PowerShell tools you have available: PS> help printer Name Category Module Synopsis ---- -------- ------ -------- Add-Printer Function PrintManagement Adds a ... 3 Your echo "multi-line input" | fdisk /dev/sda command is horrendous to read and maintain. The Arch Linux wiki page about partitioning states: fdisk (util-linux) fdisk(8) – Dialog-driven program for creation and manipulation of partition tables. cfdisk(8) – Curses-based variant of fdisk. sfdisk(8) – Scriptable variant of fdisk. Clearly, ... 3 Welcome to Code Review! It is a good practice if you start verifying your shell scripts against shellcheck. Currently, it lists quite a few issues: Line 3: SCRIPTNAME=basename "$0" ^-- SC2006: Use $(...) notation instead of legacy backticked .... Line 17: DIRBASHRCROOT="$HOME"/.bashrcroot ^-- SC2034: DIRBASHRCROOT appears unused. Verify use ...

3

To sudo or not to sudo I'm also wondering if it's better to only run the commands that need it with sudo or if I should rerun the whole script as sudo after prompting for the password like this: if [[ "$EUID" != 0 ]]; then warning "Please run as root" sudo "$0" "\$@" fi Can the script do its job without root permissions? This script cannot, ...