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

Prepend 'launcher' to commands executed with 'heroku run:inside' #3116

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

sbosio
Copy link
Contributor

@sbosio sbosio commented Dec 2, 2024

Description

Changes implemented here are better described on this Slack thread (Heroku internal).

TL;DR we're providing heroku run:inside for customers on Fir Pilot to replace heroku run which isn't supported on Fir apps yet, to run arbitrary commands on a dyno. For CNB stack apps (all Fir apps, currently), commands provided by a buildpack or depending on configs set by a buildpack need to be run through an entrypoint script called launcher). Here we first check the app stack and if it's a CNB app, we prepend the string launcher to the command being invoked by default.

We also change some copy texts to the approved CX strings, remove the hidden property for this command to be exposed and add a --no-launcher flag that allows to opt-out from the automatically added prefix for some edge use cases.

Testing

Pre-work

For running these tests you will need the latest beta version for Heroku CLI (heroku update beta) and both a deployed app to a Cedar (Dogwood) Private Space and another one deployed to a Fir space, with active dynos. I did the following for preparation (thanks @justinwilaby for letting me use your test spaces):

  • Create a Cedar PS app: heroku apps:create test-run-inside-cedar --space justins-dev-space.
  • Create a Fir PS app: heroku apps:create test-run-inside-fir --space justins-dev-fir-space.
  • Clone the Node.js Getting started app: git clone [email protected]:heroku/node-js-getting-started.git.
  • Added remotes for both apps to the same codebase: cd node-js-getting-started && heroku git:remote -a test-run-inside-cedar -r cedar && heroku git:remote -a test-run-inside-fir -r fir.
  • Deploy the code to the cedar app: git push cedar main, wait for build and release to finish.
  • Repeat with the fir app: git push fir main.

Actual testing

  • Get the dyno name for the cedar app: heroku ps -a test-run-inside-cedar, it should always be web.1.
  • Get the dyno name for the fir app: heroku ps -a test-run-inside-fir, for me it was web-55695d5c49-n9sc2, but it'll change for you, just grab whatever the dyno name is.
  • Attempt to run a command inside the cedar app dyno: heroku run:inside web.1 -a test-run-inside-cedar -- echo test. It should error out saying the command is unavailable for that app (it requires a special flag for cedar apps).
  • Flag the app to allow run:inside: heroku labs:enable dyno-run-inside -a test-run-inside-cedar.
  • Retry the same command: heroku run:inside web.1 -a test-run-inside-cedar -- echo test. Now it should work and you should see the message test.
  • Now try the same command in the fir app: heroku run:inside web-55695d5c49-n9sc2 -a test-run-inside-fir -- echo test. It should run prepending launcher to the actual command and it should work.
  • Now try the same command in the fir app with option --no-launcher: heroku run:inside web-55695d5c49-n9sc2 -a test-run-inside-fir --no-launcher -- echo test. It should run without prepending launcher to the command and it should work.

A caveat, though, there seems to be an issue with the launcher script when using double quotes around the command arguments, this doesn't work on the dyno:

$ ./bin/run run:inside web-55695d5c49-n9sc2 -a test-run-inside-fir -- echo "this is a test"
Running launcher echo "this is a test" on ⬢ test-run-inside-fir... up, web-55695d5c49-n9sc2
echo: eval: line 1: unexpected EOF while looking for matching `"'
echo: eval: line 1: unexpected EOF while looking for matching `"'
 is a 

Using --no-launcher makes work, but the response isn't the expected one either:

$ ./bin/run run:inside web-55695d5c49-n9sc2 -a test-run-inside-fir --no-launcher -- echo "this is a test"
Running echo "this is a test" on ⬢ test-run-inside-fir... up, web-55695d5c49-n9sc2
"this is a test"

(notice the quotes were included in the output, when they shouldn't)

This is a failure coming from the dyno, probably due to a bad parsing of arguments on the launcher script. We will notify this on the Slack thread.

SOC2 Compliance

GUS Work Item: W-17309565

@sbosio sbosio requested a review from a team as a code owner December 2, 2024 23:36
Copy link
Contributor

@eablack eablack left a comment

Choose a reason for hiding this comment

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

Code makes sense to me. One minor comment. When testing, it does add launcher when it's supposed to, but it hangs for my fir app. Dunno if this is a CLI problem though:

Screenshot 2024-12-03 at 9 39 45 AM

packages/cli/src/lib/run/helpers.ts Outdated Show resolved Hide resolved
@sbosio sbosio merged commit 7509607 into prerelease/10.0.0-beta Dec 3, 2024
8 checks passed
@sbosio sbosio deleted the sbosio/run-inside-launcher branch December 3, 2024 19:29
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