implement ch-image modify

[reidpr]

Closes #1400.

[reidpr]

@qwofford, what do you think of this proposal?

Alternate names include deform, edit, and mutate.

It would be nice not to implement the save prompt, i.e., only have the behavior with --yes as currently written and save or not depending on shell exit status.

[qwofford]

I like this proposal. I hadn't considered the implications of running concurrent interactive sessions on the same deployed container. The note in the documentation about an explicit need for -o DEST in order to truly modify a deployed container is clear. Thumbs up from me...I know @azukaitis is interested in this feature too.

[qwofford]

I think this might be a slightly greedy hope...but in reference to the warning block of text here...it would be really nice (and unique among container runtimes?) if we somehow stored a series of Dockerfile commands that represents the build cache too. Anything from a Dockerfile could be stored directly, and then anything from a ch-image modify would get pre-pended with RUN. That way you could build up a container interactively, then do something like a ch-image to-dockerfile DEST and snip out any commands that weren't ultimately important.

This could get us the interactive functionality we want without diverging too far from best practices at the end of the day.

[reidpr]

if we somehow stored a series of Dockerfile commands that represents the build cache too

This is a cool idea. I can think of a couple approaches:

1. Horrible, nearly useless shell: Charliecloud interactively collects a command line, then executes it. Simple to implement, but loses essentially all shell features invented since 1975.

2. Munge Bash history variables and commands: Run a normal Bash shell, which can capture its own command history. Set that up so we can translate that into a Dockerfile. This would only work with specifically supported shells. I don't know how hard it would be to do this.

[lucaudill]

@reidpr I'm ready for you to have a look at my progress so far

[reidpr]

Nice work! I think next steps are:

1. Write docs describing the behavior we've discussed and you've implemented, including particularly the different behaviors depending on where the input is coming from.
2. Make the cache behavior like import.

[reidpr]

I think you probably want nargs=1 (the default?) and just repeat the option for multiple values (action="append" does this). nargs="+" and nargs="*" can cause problems though I don't recall the details.

[reidpr]

You can tell argparse to put the value of --out in tag when you declare the command line option. That seems reasonable to me. Or, maybe do the reverse: rename cli.tag to cli.out?

[reidpr]

I wonder if the -c, interactive stdin, and non-interactive stdin cases are sufficiently different that they should be dispatched to different functions? In any case, we should explain the situation and why the decisions made were made.

[reidpr]

This won’t work due to the shell’s complicated rules about line breaks. I’d treat the input as an opaque blob to pass on to the shell’s stdin.

[reidpr]

yeah that's probably essential for merging

[reidpr]

The need for this will go away if you remove nargs="*".

[reidpr]

Regarding exit codes, I analyzed all man pages in §1 and §8 on man7.org to find codes that were rarely used by other software (e.g., code 0 is documented by 464 man pages and code 122 by none).

curl(1) documents the most exit codes (0–99) by quite a lot. Notably, it also says that “[m]ore error codes might appear here in future releases”. Whether or how fast this is happening, I don’t know (Git version history was hard to parse).

Thus, a plausible summary of unused or rarely-used exit codes has two parts.

Exit codes used only by curl(1)

There are quite a few, but the ones that seem most obscure to me, and thus least likely to cause a collision in practice, are:

code	description (quoted)
31	FTP could not use REST. The REST command failed. This command is used for resumed FTP transfers.
33	HTTP range error. The range "command" did not work.
49	Malformed telnet option.
84	The FTP PRET command failed
87	Unable to parse FTP file list.
88	FTP chunk callback reported error. (edit: can be anti-semetic)

Exit codes not documented by any man page

100–122 inclusive.

Exit codes ≥128

These are commonly used by shells and other programs that run subprocesses to indicate the child was killed by a signal. E.g. su(1): “If the command was killed by a signal, su returns the number of the signal plus 128”.

[reidpr]

Solid. In particular the clear, well-written comments continue.

I think my main criticism is that the logic of selecting the mode is mixed in with carrying out that mode. Is there a way to just (1) figure out the mode, then (2) do the setup (build tree or run interactive session, then (3) call build.main() or misc.import()? Possibly in a new modify.py?

Also I suspect you should validate (a) cache behavior and (b) parse trees.

[reidpr]

Great work; thanks Lucas. A few nits but I’m not sure it’s worth holding up merge over those?

[reidpr]

Suggested change

	elif (sys.stdin.isatty() or (cli.interactive)):
	elif (sys.stdin.isatty() or cli.interactive):

[reidpr]

Helper function might be clearer here?