Evolve ResolveVars into ResolveProgram

[sourishkrout]

Supersedes #508. "Program" as in "Cell Shell Program".

Notable changes are:

• Service solely decides the resolution/prompt status of variables. Based on the mode mode passed by the client.
• Only top-level export vars are being considered. We don't want to interfere with anything tangled up in flow control.
• Service rewrites scripts (returned as command sequence) to remove unresolved vars to be passed as part of ENV store.
• Integrated with extension: Use ResolveProgram in Runner service for prompts vscode-runme#1153
• Integration with CLI (intentionally pending).
• Removing option to pass commands since ResolveProgram (intentionally pending) will be responsible for turning shell scripts into commands sequence (skipped for shebang).

In a future version, a script to command normalization will be included, too. However, we can wait until further progress is made on the runnerv2alpha1 refactor. Service is ported to runnerv2. However, the tests are somewhat incomplete (todos).

Example: https://us-central1.stateful.com/cell/clsyzdcv4000088eiccrten9s

[sourishkrout]

I'm going to merge this on a successful build @adambabik. It'd still be interested in your review, please.

While it's not immediately apparent, this work is a precursor for what I call the "smart env store".

[sonarcloud]

Quality Gate failed

Failed conditions
66.8% Coverage on New Code (required ≥ 70%)

See analysis details on SonarCloud

[pastuxso]

LGTM 🚀

[adambabik]

@sourishkrout this is hard to understand conceptually what Prompt really is. It seems like it's an indicator how the prompt in the VS Code should look like, based on the mode and whether an env is found in the sources. Is this correct?

If so, I would rename ProgramResolverPrompt to ProgramResolverStatus and change enum values to:

• ProgramResolverStatusResolved meaning that the env is resolved and no further action is needed.
• ProgramResolverStatusUnresolvedWithMessage meaning that the env is unresolved but there is a message to display.
• ProgramResolverStatusUnresolvedWithPlaceholder meaning that the env is unresolved but there is a placeholder/default value.

[sourishkrout]

Correct. I actually had it called Status before and renamed it to Prompt but can't remember the reason for it.

The renaming makes sense and is definitely more self-describing 👍 .

[adambabik]

Overall, I think the implementation would be easier to reason about if ProgramResolver didn't contain mode. Instead, there would be ProgramResolverWithMode which would look like this:

struct ProgramResolverWithMode {
  *ProgramResolver
  Mode ProgramResolverMode
}

[sourishkrout]

Gotcha. Works for me 👍.

[adambabik]

Using \n as a separator is not safe when we want to reconstruct the command list back. Within the script there might be escaped \n which here will not be properly handled. Likely the Resolve method need to work with a script and individual lines, which all should be valid shell commands, right?

[sourishkrout]

Yep, you're right if, eg a heredoc was gonna be inside of the script the splitting would garble it.

The reason why I'd like to return a list of commands here is to make it explicit in the API that normalization/transformation from script->commands has already happened. However, I'm clearly not handling this well.

Feel free to propose a solution @adambabik.

[sourishkrout]

Interestingly, it actually works! Please see: https://us-central1.stateful.com/cell/clt0fkcs6000288eiz1b9nbnu

I believe the reason is because the splitting happens on a string-level as opposed to the raw bytes where \\n vs \n are unescaped appropriately. Wdyt @adambabik?

[adambabik]

This is an interesting case but I don't quite understand how Go processes escaped \n in this case.

I was thinking about a bit simpler example, however, I am not sure if this is how the client will use this method. The current definition allows such usage, though.

Check out this test.

Basically, with a program as follows:

export FILE_NAME=default.txt
cat >"$FILE_NAME" <<EOF
Some content with\nnew line
EOF

I expect the output to be:

#
# FILE_NAME set in smart env store
# "export FILE_NAME=default.txt"

cat >"$FILE_NAME" <<EOF
Some content with\nnew line
EOF

Instead, I got (+/- new line at the end):

#
# FILE_NAME set in smart env store
# "export FILE_NAME=default.txt", 

cat >"$FILE_NAME" <<EOF
Some content with
new line
EOF

Also, the question is why ResolveProgramResponse does not contain the source in the same format as in the input but always return a list of commands. Should that be fixed?

[sourishkrout]

Yes, you're right. You managed to "reproduce" the problem. The only thing I can think of is that on the extension side \n is being escaped as \\n. I can x-check.

@adambabik perhaps it'd be better to use the SingleLine option and "line-break" the commands at ; instead?

[sourishkrout]

@adambabik looks like the \\n is why it "works" ☝️ but I still don't quite get it

[sourishkrout]

Here's the same buf encoded as string printed with quotes (%q):

"cat >somefile <<EOF\nline1\nline2\\n\\n\nline3\nEOF\ncat somefile\n"

So I'd say that in the test case you're referencing the new-lines aren't escaped properly. However, perhaps that's another reason to get away from \n line breaking.

[adambabik]

In terms of the solution. Currently, the service accepts two kinds of program source: (1) a script as a single blob, (2) a list of commands. My first question would be whether we need both and in which cases the client uses one vs another.

The case of a script as a blob is easy, provided that the service returns it as a blob back. This is not the case now. Any reason for it?

For the case of a list of commands, the question is whether each line is a separate valid shell program. If not, then we need to concatenate it before processing and then split back. This is problematic. I will check if we can use something more complex than \n to do concatenate-and-split that our shell parser is ok with. We can also split it using a custom algorithm as we know the input and know how it can be modified, but I would prefer to avoid it.