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

Add tunnel_options in ssh_generator #176

Merged
merged 3 commits into from
Oct 21, 2021

Conversation

nishchintraina
Copy link
Contributor

@nishchintraina nishchintraina commented Oct 11, 2021

Context in the open issue #175

Testing

  • Make sure the tunnel options are picked up
$ time taste-tester -vvvv --<redacted> --solo <redacted> test -s <redacted_host> --skip-repo-checks --yes
Taste-tester type: fb-chef-solo[fbinstance] (CLOUD)
Loading plugin at /etc/taste-tester/hooks/cloud-hooks.rb
Using /home/nish/opsfiles
Trying to detect repo type
Repo found to be Hg
Running "hg log -r . -l 1 -T '{node}'"
Running "hg log -r . -l 1 -T '{desc}'"
Running "hg log -r . -l 1 -T '{author}'"
.
.
Running post_upload hook, skip with --skip-post-upload-hook
Uploading local chefctl and chef_bootstrap
Trying to detect repo type
Repo found to be Hg
Taste-testing on <redacted_host>
Will run: '( [ -s /opt/chef-solo/<redacted>/test_timestamp ] && kill -9 -- -$(cat /opt/chef-solo/fbinstance/test_timestamp) 2>/dev/null;  true )' on <redacted_host>
Running: TERM=screen.xterm-256color SSH_AUTH_SOCK=/tmp/cloud-ssh-nish-agent /bin/ssh -T -p 22 -o ServerAliveInterval=60 -o ServerAliveCountMax=5 -o '<redacted_options>' centos@<redacted_host> "echo 'KCBbIC1zIC9vcHQvY2hlZi1zb2xvL2ZiaW5zdGFuY2UvdGVzdF90aW1lc3RhbXAgXSAmJiBraWxsIC05IC0tIC0kKGNhdCAvb3B0L2NoZWYtc29sby9mYmluc3RhbmNlL3Rlc3RfdGltZXN0YW1wKSAyPi9kZXYvbnVsbDsgIHRydWUgKQ==' | base64 --decode | sudo bash -x"
STDERR: /opt/chefdk/embedded/lib/ruby/gems/2.5.0/gems/mixlib-shellout-2.4.4/lib/mixlib/shellout/unix.rb:340: warning: Insecure world writable dir /var/www/scripts in PATH, mode 040777
STDERR: Warning: Permanently added '<redacted_host>' (ECDSA) to the list of known hosts.
STDERR: + '[' -s /opt/chef-solo/fbinstance/test_timestamp ']'
STDERR: ++ cat /opt/chef-solo/fbinstance/test_timestamp
STDERR: + kill -9 -- -3166345
STDERR: + true
Setting up tunnel on port 4001
Running: TERM=screen.xterm-256color SSH_AUTH_SOCK=/tmp/cloud-ssh-nish-agent /bin/ssh -T -p 22 -o ServerAliveInterval=60 -o ServerAliveCountMax=5 -o '<redacted_options>' centos@<redacted_host> -f -R 4001:localhost:5181 ....

Copy link
Collaborator

@jaymzh jaymzh left a comment

Choose a reason for hiding this comment

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

This seems good to me. I'm bummed I didn't catch all the contexts and tests being named wrong. If you're up for fixing them, that'd be great, but I wont' block this on that. Or do another PR or whatever.
I'll give this a day or two for @NaomiReeves or @davide125 to review this before I (or they) merge it.

allow(mock_so).to receive(:stdout).and_return(mock_generated_cmd)
end

it 'test ssh generated command' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor nit, the tests (the it statements) should be named for what the thing is supposed to DO. So it should be like it "generates commands". Meanwhile the "contexts" should be "when" statements to describe the context. because then the output is like:

* when using custom generators
  * it generates a proper non-tunnel command
  * it generates a proper tunnel command

Copy link
Contributor Author

@nishchintraina nishchintraina Oct 13, 2021

Choose a reason for hiding this comment

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

Oh! lol I guess I should...

Alt Text

@nishchintraina
Copy link
Contributor Author

This seems good to me. I'm bummed I didn't catch all the contexts and tests being named wrong. If you're up for fixing them, that'd be great, but I wont' block this on that. Or do another PR or whatever. I'll give this a day or two for @NaomiReeves or @davide125 to review this before I (or they) merge it.

I will pull separate PR for this - fix all specs. :)

@nishchintraina
Copy link
Contributor Author

This seems good to me. I'm bummed I didn't catch all the contexts and tests being named wrong. If you're up for fixing them, that'd be great, but I wont' block this on that. Or do another PR or whatever. I'll give this a day or two for @NaomiReeves or @davide125 to review this before I (or they) merge it.

I will pull separate PR for this - fix all specs. :)

#177 for tracking this

@nishchintraina
Copy link
Contributor Author

catping :)

@NaomiReeves NaomiReeves merged commit b0f3410 into facebook:main Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants