Make various fixes throughout the code, use separate logging using Python logging #47
Conversation
Factor out checking for root privileges into a separate utility function. Also ensure "snf-image-creator --help" runs successfully without root privileges.
Port dialog_main.py to use argparse instead of optparse. Add explicit positional argument, improve help text for every argument, add description and epilog blocks to output of --help.
Media is the plural of medium. This program only supports a single medium at a time, a raw hard disk, or a disk image file, so `medium' is appropriate.
Make the process of resetting the terminal and outputting the text of an unexpected exception to stderr more robust. It seems resetting the terminal after using dialog races with printing the text of the exception, and overwrites it with the dialog background. Sleep for a tiny amount of time to ensure the exception is visible. This is an ugly workaround and should be replaced.
Re-write the procedure of selecting a directory to hold temporary files, so it does not use an arbitrary list of directories including /mnt and the user's home directory (which is presumably /root), but starts with a list of all mountpoints and /tmp, /var/tmp as candidates. Also fix bug where the creator would choose a directory under a read-only mount for storing temporary files, and fail. Still, disable this code path temporarily, and only choose between /tmp and /var/tmp, if the user has made no explicit choice. It is very un-intuitive to create temporary files in unrelated directories without the user's explicit consent. Perhaps this code path can be enabled again when snf-image-creator has the ability to propose these alternate locations to the user, and have them choose explicitly.
Separate the process of logging from the process of interacting with
the user ("Output" instances) to ensure emitting debugging information
is possible from anywhere inside the code.
Use the standard Python logging module for logging, instead of a custom
approach. This brings lots of advantages, including proper log
formatting and timestamping.
Use logging.handlers.SysLogHandler for logging to syslog,
instead of calling the syslog module directly.
Also use a standard default logfile /var/log/snf-image-creator
for persistence, instead of a temporary logfile, and remove redundant
functions show_log(), copy_file(). This simplifies the error handling
path significantly.
The general idea of this commit is that the instances of "Output" should
be independent frontends for interacting with the user, and can be
extended to support Input as well as Output in the future.
This is a Work in Progress. The previous code assumed that all messages
to the user via Output instances would also make it to the log file.
For now, only what is explicitly logged reaches the logfile.
A workaround is to have the base Output class log all messages to the
user, and will be implemented in a future commit.
|
Hello, That's a very substantial pull request, thank you! To be able to incorporate parts or all of it to Synnefo, if the work is not already under GRNET's copyright, we will need to have the appropriate rights, preferably through a Copyright License Agreement (CLA), which is the norm in large projects accepting contributions. A CLA is essential to ensure the users of the software that they need not worry about the continuity of the code and they are guaranteed against any potential future problems. See for example the FAQ at the Django CLA. |
|
Thank you Panos. Yes, I agree completely: Since my contribution is not under GRNET's copyright, it's important to ensure continuity of the code. The Django CLA is fine. Will you provide me with a version of it for GRNET, so we can proceed? |
|
We'll draft a GRNET CLA ASAP and send it over to you. |
|
Please find the CLA at: https://www.synnefo.org/assets/grnet_cla.txt Sign, scan, and send to [email protected] so that we can use the contribution. |
|
Panos, I have read the CLA. I see two differences from the Apache CLA: The GRNET CLA reads Given that I retain Copyright for my contribution to snf-image-creator, If the above issue is sorted out, I can make another pull request fixing the minor issues pointed out by Nikos. Also, the phrase "In return, the Foundation shall not use Your Contributions in a way that is contrary to the public benefit or inconsistent with its nonprofit status and bylaws in effect at the time of the contribution." is missing, but I don't have a problem with this missing part. Regarding style, do I leave my added Copyright line in every new/modified file, the way OpenStack does, or does it suffice to change the line to "Copyright GRNET and contributors" in new/modified files and in the main README.md and just have my name in AUTHORS, like Django does? |
|
|
Panos, About the first part, thanks, please update the CLA. About replacing my Copyright with GRNET's, what do you mean? You cannot remove the Copyright line, since I retain the Copyright. The point is whether this fact is displayed in individual files, or in the single Copyright line in the root README.md. |
|
We updated the CLA. Regarding the copyright notice, you grant GRNET a non-exclusive copyright license. Non-exclusive means that GRNET can exercise the right in question, i.e., copyright, but so can you, too. That's in contrast to an exclusive license, which would mean that only GRNET would have the right in question from now on. As explained above, the modus operandi is exactly equivalent to say, Google and Ganeti. It's only the Google copyright that appears in Ganeti contributions, not individual contributors' (neither internal not external). We may have contributors listed in a related file, not copyright holders in our source files, apart from GRNET. |
|
Panos, I am not a lawyer, but your comment does not hold. This is also explicitly mentioned No. This is a pure license agreement, not a copyright assignment. You still maintain the full copyright for your contributions. You are only providing a license to the DSF to distribute your code without further restrictions. This is not the case for all CLA's, but it is the case for the one we are using. Regarding the modus operandi: Google also acknowledges this fact, and makes it explicit, for example in the AUTHORS file for Go: and also uses the proper Copyright line everywhere: Both of these projects, and many others, like OpenStack, use the same Apache / Google CLA. Also, please note how the AUTHORS file of Go mentions the Copyright implications So, either we list all contributors individually, as I have done in this pull request, |
|
IANAL either, but here is what I do understand.
In our specific case now. Needless to say we have every reason to welcome contributors and to show appreciation for their work, that's what Open Source is all about. Adding contributors by name in each and every file is a no-go, we might end up with dozens, very ugly. Also what will happen if in a future commit we factor away some contributors' code? We cannot be making rounds in committed code to see who is still a contributor to each file. So we can do the following:
Note that the choice of Google in naming files is very poor, it has authors and contributors the wrong way round. |
|
OK, Panos, thank you. I will sign the CLA, send it to you, and remake the pull request incorporating the agreed changes. |
Make various fixes throughout the code.
Convert argument-parsing to python-argparse, improve help messages.
Use separate logging procedure, separate it from interacting with the user (Output classes).
Use it to introduce the support for debugging messages throughout the code.
Use a persistent logfile, ensure all errors are logged.
This is a work-in-progress.
The end goal is to unify all frontends into a single framework:
To create an Image is to run a series of steps. Each step has input, output, and depends on other steps. Each step modifies global state (image context). Create a single framework for executing steps, which will be able to detect dependencies between the steps, and run them either in an unattended fashion, or with manual interaction.
Finally, make the functionality of Output classes completely distinct, so a single codebase can run either with manual intervention and a dialog-based frontend (the current snf-image-creator), or with minimal intervention and a text-based frontend (the current snf-mkimage), or any other combination in between.