Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement "runme beta session" #683

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Implement "runme beta session" #683

wants to merge 8 commits into from

Conversation

adambabik
Copy link
Collaborator

It implements a new command that allows starting a session and collecting all exported environment variables throughout its lifetime.

Testing

As a prerequisite, add the following to your shell's startup script (.bashrc or .zshrc):

# runme
source <(runme beta session setup)

Then, you can start a new session in the terminal:

runme beta session

The session is a regular shell of your choice, inferred from $SHELL. You can use it like a normal shell, but any exported environment variables will be returned upon exiting the session via exit.

Currently, the collected variables are printed out, but in the future, they will be routed to a proper store.

@adambabik adambabik changed the title Implement "beta session" Implement "runme beta session" Oct 13, 2024
@adambabik adambabik marked this pull request as ready for review October 14, 2024 20:22
@adambabik
Copy link
Collaborator Author

7351fbf can be cherry-picked to main safely. After one of the GitHub Actions update, the CI workflow does not work.

Copy link
Member

@sourishkrout sourishkrout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this is the right direction. Essentially keeping it simple. See comments, please.

@sourishkrout
Copy link
Member

sourishkrout commented Oct 16, 2024

I'm getting this locally on macOS with zsh:

# runme
source <(runme beta session setup)
/dev/fd/14:3: parse error near `\n'

Same happens when I source it inside of .zshrc.

@adambabik
Copy link
Collaborator Author

I'm getting this locally on macOS with zsh:

# runme
source <(runme beta session setup)
/dev/fd/14:3: parse error near `\n'

Same happens when I source it inside of .zshrc.

@sourishkrout can you paste the output of executing runme beta session setup, please?

@sourishkrout
Copy link
Member

sourishkrout commented Oct 16, 2024

@sourishkrout can you paste the output of executing runme beta session setup, please?

here you go:

$ runme beta session setup
#!/bin/sh
/Users/sourishkrout/Projects/stateful/oss/runme/runme env dump --insecure > 
__cleanup() {
rv=$?
/Users/sourishkrout/Projects/stateful/oss/runme/runme env dump --insecure > 
exit $rv
}
trap -- "__cleanup" EXIT

oh, I see the problem now... no destinations for the redirects

@adambabik
Copy link
Collaborator Author

oh, I see the problem now... no destinations for the redirects

Thanks! Yeah, I will add a check to protect from this... Overall, RUNME_SESSION=1 is required to activate the session setup. Then, two envs from the env collector that define those redirects are used. This is all hidden from the user when using runme beta session, but these edge cases should be handled.

@sourishkrout
Copy link
Member

It does not work for me. Can you provide step-by-step instructions on how to start with a clean slate? How are you running this locally?

I mainly don't understand how the setup command will work without RUNME_SESSION_PREPATH and RUNME_SESSION_POSTPATH present or how RUNME_SESSION=1 need to be 1?

Isn't this a chicken/egg problem @adambabik?

@sourishkrout
Copy link
Member

I might misunderstand the purpose of session setup 🤔. That's probably why it's not "working" for me.

@adambabik
Copy link
Collaborator Author

@sourishkrout sorry, I missed your comment. It should work exactly as it's described in the PR description. In my case:

  1. I put source <(/Users/myuser/path/to/runme/runme beta session setup) where runme is a binary build in the project root directory on this branch.
  2. Do ./runme beta session and it should display Runme session active. When you're done, execute "exit"..
  3. Export some env like export TEST=1
  4. Type exit. It should print:
Collected env during the session:
TEST=1

All the shenanigans with the RUNME_ envs are handled by beta session setup and beta session.

@sourishkrout
Copy link
Member

Collected env during the session:
TEST=1

It shows empty for me following the exact instructions 🤔. Not sure why, @adambabik.

Comment on lines +145 to +152
if err := requireEnvs(
command.EnvCollectorSessionEnvName,
command.EnvCollectorSessionPrePathEnvName,
command.EnvCollectorSessionPostPathEnvName,
); err != nil {
logger.Info("session setup is skipped because the environment variable is not set", zap.Error(err))
return writeNoopShellCommand(out)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I don't get is how these won't ever produce an error? In other words how are they set in the first place if they are expected by setup @adambabik?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order is that beta session starts a shell process with those envs provided first. This shell process starts and executes, in the case of zsh, the .zshrc file inside which beta session setup is called.

It's perfectly normal that it typically will error and output no-op shell command in .zshrc, but from the user point of view it should be invisible. We can recommend protecting source <(/path/to/runme beta session setup) with a conditional checking command.EnvCollectorSessionEnvName to avoid that.

Comment on lines +154 to +158
sessionSetupEnabled := os.Getenv(command.EnvCollectorSessionEnvName)
if val, err := strconv.ParseBool(sessionSetupEnabled); err != nil || !val {
logger.Debug("session setup is skipped", zap.Error(err), zap.Bool("value", val))
return writeNoopShellCommand(out)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is RUNME_SESSION expected to be a bool (serialized)? We are using the env var to hold the session ID all over the place so a semantic clash here is bad if bool <> SessionID (both string).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, that's a good point. I didn't notice the clash. I will change it to something like RUNME_TERM_SESSION_ENABLED.

@adambabik
Copy link
Collaborator Author

Collected env during the session:
TEST=1

It shows empty for me following the exact instructions 🤔. Not sure why, @adambabik.

In order to debug it, try tail -f /tmp/runme.log in another terminal window and then e.g. source ~/.zshrc or opening a new terminal window. The path is taken from experimental/runme.yaml. I noticed that it's too chatty for some logs but overall should contain an answer.

Copy link

sonarcloud bot commented Nov 9, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
32.1% Coverage on New Code (required ≥ 50%)

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants