Repository navigation
chore(ci): replace toys kokoro-ci with a Ruby script - #1910
torreypayne wants to merge 5 commits into
Conversation
bin/kokoro-ci is a plain-Ruby port of .toys/kokoro-ci.rb that needs only default gems, so this repo no longer depends on the toys gem in the Kokoro image. .kokoro/system_tests.sh now runs `ruby bin/kokoro-ci -v`. The steps, commands and exit codes are unchanged. bin/ joins the paths whose changes run every test, so edits to this script get a full presubmit run.
Nothing calls .toys/update-gemfile-locks.rb (no Kokoro config, workflow or script refers to it). It was the last file in .toys/, so bin/kokoro-ci no longer lists .toys/ among the paths whose changes run every test.
There was a problem hiding this comment.
Code Review
This pull request replaces the toys kokoro-ci tool with a pure Ruby script bin/kokoro-ci that runs using only default gems, updating .kokoro/system_tests.sh and removing the old toys tasks. The review feedback highlights three key issues in the new script: a potential resource leak during cleanup if gimmeproj done fails, an unhandled Errno::EPIPE exception when writing to a crashed Ruby process pipe, and a copy-paste bug that checks for PostgreSQL instead of SQL Server when starting the SQL Server proxy.
| # If setup failed, a failing `gimmeproj done` still exits immediately and | ||
| # skips the kills, as it did in the toys tool. | ||
| def run_cleanup | ||
| run ["gimmeproj", "-project", "cloud-samples-ruby-test-kokoro", "done", @project] unless @project.empty? | ||
| @kill_on_cleanup.each do |pid| | ||
| Process.kill "SIGKILL", pid if pid | ||
| rescue StandardError | ||
| nil | ||
| end | ||
| end |
There was a problem hiding this comment.
If gimmeproj done fails during cleanup, the script will exit immediately because @exit_on_failure is still true (if setup failed). This prevents the subsequent background processes in @kill_on_cleanup from being killed, leading to leaked resources on the CI runner. Setting @exit_on_failure = false at the start of run_cleanup ensures that all cleanup steps are executed.
# Ensure background processes are always killed even if gimmeproj fails.
def run_cleanup
@exit_on_failure = false
run ["gimmeproj", "-project", "cloud-samples-ruby-test-kokoro", "done", @project] unless @project.empty?
@kill_on_cleanup.each do |pid|
Process.kill "SIGKILL", pid if pid
rescue StandardError
nil
end
end| def run_ruby args, script | ||
| reader, writer = IO.pipe | ||
| pid = start_process [RbConfig.ruby, *args], argv0: "ruby", log: "exec ruby: #{args.inspect}", in: reader | ||
| reader.close | ||
| writer.write script if pid | ||
| writer.close | ||
| check_result pid && Process.wait2(pid).last | ||
| end |
There was a problem hiding this comment.
If the spawned Ruby process exits or crashes before reading the script from the pipe, writer.write will raise Errno::EPIPE (broken pipe). Since this exception is unhandled, it will crash the entire CI run instead of marking the individual test as failed. Wrapping the write and close operations in a begin ... rescue Errno::EPIPE ... ensure block prevents this crash and allows the script to correctly capture the process exit status.
def run_ruby args, script
reader, writer = IO.pipe
pid = start_process [RbConfig.ruby, *args], argv0: "ruby", log: "exec ruby: #{args.inspect}", in: reader
reader.close
begin
writer.write script if pid
rescue Errno::EPIPE
# Child process exited before reading the script
ensure
writer.close rescue nil
end
check_result pid && Process.wait2(pid).last
end| # The toys tool checked for cloud-sql/postgres here, not cloud-sql/sqlserver. | ||
| # Kept as is: these cloud-sql directories are never products today, because | ||
| # their tests live one level deeper (e.g. cloud-sql/mysql/activerecord). | ||
| if @products.include? "cloud-sql/postgres" | ||
| say "Starting Cloud SQL Proxy for SQL Server", :bold | ||
| connection_name = assert_env "SQLSERVER_INSTANCE_CONNECTION_NAME" | ||
| pid = run_background ["/bin/cloud_sql_proxy", | ||
| "-instances=#{connection_name}=tcp:1433", | ||
| "-credential_file=#{gac_path}"] | ||
| ENV["SQLSERVER_CLOUD_SQL_PROXY_PROCESS_ID"] = pid.to_s | ||
| @kill_on_cleanup << pid | ||
| end |
There was a problem hiding this comment.
This is a copy-paste bug carried over from the original toys tool. It checks for cloud-sql/postgres to start the SQL Server proxy, which means the SQL Server proxy is started unnecessarily when running Postgres tests, and would not be started if only SQL Server tests were run. We should check for cloud-sql/sqlserver instead.
if @products.include? "cloud-sql/sqlserver"
say "Starting Cloud SQL Proxy for SQL Server", :bold
connection_name = assert_env "SQLSERVER_INSTANCE_CONNECTION_NAME"
pid = run_background ["/bin/cloud_sql_proxy",
"-instances=#{connection_name}=tcp:1433",
"-credential_file=#{gac_path}"]
ENV["SQLSERVER_CLOUD_SQL_PROXY_PROCESS_ID"] = pid.to_s
@kill_on_cleanup << pid
endIf setup failed, a failing `gimmeproj done` exited the script before it killed the background processes, as the toys tool did. Cleanup now turns off exit-on-failure first, so every step runs and the setup failure keeps its exit status.
run_ruby writes the test script to Ruby's stdin. If Ruby exited before it read the whole script, the write raised Errno::EPIPE and ended the whole run, as the toys tool's controller write did. The write now ignores EPIPE, so Ruby's exit status marks the test as failed and the run continues.
The check copied from the toys tool tested for cloud-sql/postgres. It now tests for cloud-sql/sqlserver, which matches the proxy it starts. Today's runs do not change, because no cloud-sql directory is a product: their tests live one level deeper.
Part of the move off toys (b/563024235).
bin/kokoro-ci, a plain-Ruby port of.toys/kokoro-ci.rbthat needs only default gems, and runs it from.kokoro/system_tests.sh.bin/.update-gemfile-lockstool, which empties.toys/.Parity: a harness ran both tools through 38 scenarios on Ruby 3.2 to 4.0 and recorded identical commands, environments and exit codes, apart from those fixes.