fix(docker): safer argument parsing for GitHub Actions #788

kanadgupta · 2023-03-31T00:48:52Z

🚥 Fixes #787

🧰 Changes

The Problem 😵‍💫

Someone wrote in in #787 about an issue where their workflows started breaking due to them wrapping their file names in quotation marks. Normally our args parser (command-line-args) would handle quotation marks just fine, but because of our new Docker setup and how we have the user pass in all their rdme arguments into a single GitHub Actions input, quotation marks are escaped by the time rdme receives it 😬

To put it a different way, this was the equivalent of running something like this:

The Solution 💭

This was a pain to debug and fix (thanks @RyanGWU82 and @domharrington for pairing on this 😮‍💨), but I got something going that I'm happy with! Now, when we invoke the rdme Docker image via the GitHub Action runner, it's basically running this:

rdme docker-gha "openapi:validate \"https://github.com/readmeio/oas-examples/blob/main/3.0/json/petstore-simple.json\""

The docker-gha argument isn't documented nor exposed to users, but is basically an internal escape hatch so we can properly consume rdme arguments that are sent to us via a single string.

As part of this, I did the following:

Refactors our Dockerfile and our action.yml files so we're now using rdme as the executable and passing in the rdme command via good ol' fashioned docker run arguments.

To put this a different way, here's the before and after if we were to build the rdme Docker image locally and pass arguments into it:

Before (using the INPUT_RDME environmental variable):
docker run -i -t --env INPUT_RDME='openapi:validate https://github.com/readmeio/oas-examples/blob/main/3.0/json/petstore-simple.json' ghcr.io/readmeio/rdme:dev
After (using standard docker run arguments):
docker run -i -t --env DEBUG='rdme*' ghcr.io/readmeio/rdme:dev openapi:validate https://github.com/readmeio/oas-examples/blob/main/3.0/json/petstore-simple.json

Added logic our main rdme index.ts file to check for this new docker-gha command and if it exists, parse out the arguments using string-argv.
Added corresponding test coverage, both in Jest and in our GitHub Actions dry runs.

🧬 QA & Testing

If tests pass we should be good to go! If you can think of any edge cases with quotation marks and string serialization I might be missing, please let me know 🥺

To get a sense of what is going on here, feel free to run this locally:

# Build + install deps
npm ci && npm run build

# simulate the command that is being invoked by the GitHub Actions runner
bin/rdme docker-gha "openapi:validate \"https://github.com/readmeio/oas-examples/blob/main/3.0/json/petstore-simple.json\""

This reverts commit a82c278.

see this: https://stackoverflow.com/a/13802438

This reverts commit f900a39.

if this works i'm gonna scream

This reverts commit 6138dc7.

src/index.ts

kanadgupta · 2023-04-04T23:04:48Z

action.yml

@@ -14,3 +14,6 @@ outputs:
 runs:
  using: docker
  image: docker://ghcr.io/readmeio/rdme:8.6.0
+  args:
+    - docker-gha


This is a new internal argument we send to indicate to the rdme Docker image that the command is going to be a single string as opposed to an array of strings.

This is a pretty neat/sufficient solution. Annoying that it's necessary, but it's not too bad.

kanadgupta · 2023-04-04T23:05:31Z

src/index.ts

+   * This logic checks for that `docker-gha` argument and parses the second string
+   * into the arguments array that `command-line-args` is expecting.
+   */
+  if (rawProcessArgv.length === 2 && rawProcessArgv[0] === 'docker-gha') {


We now check for that docker-gha argument before parsing out the second string and converting it to something that command-line-args can process.

Hopefully this is something we can remove down the line! But nice fix for now.

Yeah sadly I think this will be here to stay so long as we aim to maintain Actions/CLI feature parity and stick with this Docker approach to Actions 🫠

But who knows, maybe GitHub will ship changes on this front that will allow us to simplify this 🤞🏽

kanadgupta · 2023-04-04T23:11:10Z

Dockerfile

@@ -8,4 +8,4 @@ FROM alpine:3.14

 COPY --from=builder /rdme/exe /exe

-ENTRYPOINT ["sh", "-c", "/exe/rdme $INPUT_RDME"]
+ENTRYPOINT ["/exe/rdme"]


Not only this this a lot cleaner, but we can now use good ol' docker run arguments AND GitHub recommends¹ accessing inputs via arguments and not environmental variables in their own docs:

Footnotes

They technically imply that it's impossible to access the INPUT_ environmental variables from the Docker container action, but that's definitely wrong — [email protected] wouldn't be working at all in that case lol. The takeaway here is that we should be passing inputs via arguments and not environmental variables. ↩

Yeah this is better.

src/index.ts

... also rename a test

erunion

hell of a bug + fix

domharrington

Good job!

domharrington · 2023-04-05T09:33:49Z

.github/workflows/ci.yml

+      - name: Run `openapi:validate` with filename in quotes
+        uses: ./rdme-repo/
+        with:
+          rdme: openapi:validate "oas-examples-repo/3.1/json/petstore.json"


Is this the previously broken case? Do you have a test for that?

Correct, both tests added to this file check for the breakage in the most recent release!

domharrington · 2023-04-05T09:33:59Z

Dockerfile

@@ -8,4 +8,4 @@ FROM alpine:3.14

 COPY --from=builder /rdme/exe /exe

-ENTRYPOINT ["sh", "-c", "/exe/rdme $INPUT_RDME"]
+ENTRYPOINT ["/exe/rdme"]


Yeah this is better.

domharrington · 2023-04-05T09:34:30Z

action.yml

@@ -14,3 +14,6 @@ outputs:
 runs:
  using: docker
  image: docker://ghcr.io/readmeio/rdme:8.6.0
+  args:
+    - docker-gha


This is a pretty neat/sufficient solution. Annoying that it's necessary, but it's not too bad.

domharrington · 2023-04-05T09:36:09Z

src/index.ts

+   * This logic checks for that `docker-gha` argument and parses the second string
+   * into the arguments array that `command-line-args` is expecting.
+   */
+  if (rawProcessArgv.length === 2 && rawProcessArgv[0] === 'docker-gha') {


Hopefully this is something we can remove down the line! But nice fix for now.

rafegoldberg

Frustrating bug; interesting fix. Changes look good to me.

RyanGWU82

Thanks Kanad!

## [8.6.1-next.1](v8.6.0...v8.6.1-next.1) (2023-04-05) ### Bug Fixes * **docker:** safer argument parsing for GitHub Actions ([#788](#788)) ([8328388](8328388)) [skip ci]

## [8.6.1](v8.6.0...v8.6.1) (2023-04-05) ### Bug Fixes * **docker:** safer argument parsing for GitHub Actions ([#788](#788)) ([8328388](8328388)) [skip ci]

kanadgupta and others added 25 commits March 30, 2023 17:48

chore: try tweaking dockerfile

a82c278

chore: try adding quotes around filename

31a276b

Revert "chore: try tweaking dockerfile"

66747e1

This reverts commit a82c278.

chore: try downgrading CLI usage

d021809

chore: try an entrypoint file approach

b5650d9

revert: rebump cliUsage

1c4b562

fix: try this

7ad9568

Merge branch 'next' into dockerfile-double-quotes

ce0dee1

chore: try this

0f8f05f

fix: get entrypoint working

6cd38a2

chore(temp): try building and pushing

7f8d7fa

chore: login oops

c3ebc04

revert: remove build push, add entrypoint debug

b6db9dc

revert: uncommnet

8c47d18

chore: try this (sigh)

9708f15

chore: reset

cec6fb6

chore: remove args oops

5cbe62d

chore: try this

f900a39

see this: https://stackoverflow.com/a/13802438

Revert "chore: try this"

58ab9cd

This reverts commit f900a39.

chore: try single quotes

6138dc7

if this works i'm gonna scream

Revert "chore: try single quotes"

544981a

This reverts commit 6138dc7.

chore: try using curly braces

ef6a8d6

chore: try this

d27bb7b

chore: wait what about this

518567b

chore: debug

8d81196

github-advanced-security bot found potential problems Apr 4, 2023

View reviewed changes

src/index.ts Fixed Show fixed Hide fixed

kanadgupta added 4 commits April 4, 2023 14:12

chore: revert

360b2fe

fix: will this work?

cbeb32c

ci: add a dedicated filename test

bf11379

ci: add another test

ac5cacc

kanadgupta added 4 commits April 4, 2023 16:04

test: add tests

282bb56

docs: add descriptive comments

a91163c

refactor: safer deterministic way to indicate GHA args

76ee1c7

chore: lint

440495b

kanadgupta changed the title ~~chore: try tweaking dockerfile~~ fix(docker): safer argument parsing for GitHub Actions Apr 4, 2023

kanadgupta commented Apr 4, 2023

View reviewed changes

kanadgupta added bug Something isn't working refactor Issues about tackling technical debt GHA / CI Issues specific to GitHub Actions or other CI environments labels Apr 4, 2023

kanadgupta requested review from erunion, RyanGWU82, domharrington, rafegoldberg and gratcliff April 4, 2023 23:12

kanadgupta marked this pull request as ready for review April 4, 2023 23:13

erunion reviewed Apr 4, 2023

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

kanadgupta added 2 commits April 4, 2023 23:01

fix: use string-argv instead

ae006db

test: add another test case

073ac68

... also rename a test

kanadgupta requested a review from erunion April 5, 2023 04:06

erunion approved these changes Apr 5, 2023

View reviewed changes

test: remove redundant tests, clean up names

054643f

domharrington approved these changes Apr 5, 2023

View reviewed changes

rafegoldberg approved these changes Apr 5, 2023

View reviewed changes

kanadgupta added 2 commits April 5, 2023 10:59

chore: add some debug statements

9fa7b48

revert: delete .DS_Store

d6c3f2f

RyanGWU82 approved these changes Apr 5, 2023

View reviewed changes

kanadgupta merged commit 8328388 into next Apr 5, 2023

kanadgupta deleted the dockerfile-double-quotes branch April 5, 2023 16:43

kanadgupta pushed a commit that referenced this pull request Apr 5, 2023

build(release): 🚀 v8.6.1 🦉

9824100

## [8.6.1](v8.6.0...v8.6.1) (2023-04-05) ### Bug Fixes * **docker:** safer argument parsing for GitHub Actions ([#788](#788)) ([8328388](8328388)) [skip ci]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(docker): safer argument parsing for GitHub Actions #788

fix(docker): safer argument parsing for GitHub Actions #788

kanadgupta commented Mar 31, 2023 •

edited

Loading

kanadgupta Apr 4, 2023

domharrington Apr 5, 2023

kanadgupta Apr 4, 2023

domharrington Apr 5, 2023

kanadgupta Apr 5, 2023

kanadgupta Apr 4, 2023 •

edited

Loading

domharrington Apr 5, 2023

erunion left a comment

domharrington left a comment

domharrington Apr 5, 2023

kanadgupta Apr 5, 2023

domharrington Apr 5, 2023

domharrington Apr 5, 2023

domharrington Apr 5, 2023

rafegoldberg left a comment •

edited

Loading

RyanGWU82 left a comment

fix(docker): safer argument parsing for GitHub Actions #788

fix(docker): safer argument parsing for GitHub Actions #788

Conversation

kanadgupta commented Mar 31, 2023 • edited Loading

🧰 Changes

The Problem 😵‍💫

The Solution 💭

🧬 QA & Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanadgupta Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

erunion left a comment

Choose a reason for hiding this comment

domharrington left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafegoldberg left a comment • edited Loading

Choose a reason for hiding this comment

RyanGWU82 left a comment

Choose a reason for hiding this comment

kanadgupta commented Mar 31, 2023 •

edited

Loading

kanadgupta Apr 4, 2023 •

edited

Loading

rafegoldberg left a comment •

edited

Loading