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

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

Merged
merged 39 commits into from
Apr 5, 2023
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
a82c278
chore: try tweaking dockerfile
kanadgupta Mar 31, 2023
31a276b
chore: try adding quotes around filename
kanadgupta Mar 31, 2023
66747e1
Revert "chore: try tweaking dockerfile"
kanadgupta Mar 31, 2023
d021809
chore: try downgrading CLI usage
kanadgupta Mar 31, 2023
b5650d9
chore: try an entrypoint file approach
kanadgupta Mar 31, 2023
1c4b562
revert: rebump cliUsage
kanadgupta Mar 31, 2023
7ad9568
fix: try this
kanadgupta Mar 31, 2023
ce0dee1
Merge branch 'next' into dockerfile-double-quotes
kanadgupta Apr 3, 2023
0f8f05f
chore: try this
kanadgupta Apr 3, 2023
6cd38a2
fix: get entrypoint working
kanadgupta Apr 3, 2023
7f8d7fa
chore(temp): try building and pushing
kanadgupta Apr 3, 2023
c3ebc04
chore: login oops
kanadgupta Apr 3, 2023
b6db9dc
revert: remove build push, add entrypoint debug
kanadgupta Apr 3, 2023
8c47d18
revert: uncommnet
kanadgupta Apr 3, 2023
9708f15
chore: try this (sigh)
kanadgupta Apr 3, 2023
cec6fb6
chore: reset
kanadgupta Apr 4, 2023
5cbe62d
chore: remove args oops
kanadgupta Apr 4, 2023
f900a39
chore: try this
kanadgupta Apr 4, 2023
58ab9cd
Revert "chore: try this"
kanadgupta Apr 4, 2023
6138dc7
chore: try single quotes
kanadgupta Apr 4, 2023
544981a
Revert "chore: try single quotes"
kanadgupta Apr 4, 2023
ef6a8d6
chore: try using curly braces
kanadgupta Apr 4, 2023
d27bb7b
chore: try this
kanadgupta Apr 4, 2023
518567b
chore: wait what about this
kanadgupta Apr 4, 2023
8d81196
chore: debug
kanadgupta Apr 4, 2023
360b2fe
chore: revert
kanadgupta Apr 4, 2023
cbeb32c
fix: will this work?
kanadgupta Apr 4, 2023
bf11379
ci: add a dedicated filename test
kanadgupta Apr 4, 2023
ac5cacc
ci: add another test
kanadgupta Apr 4, 2023
62d4014
Merge branch 'next' into dockerfile-double-quotes
kanadgupta Apr 4, 2023
282bb56
test: add tests
kanadgupta Apr 4, 2023
a91163c
docs: add descriptive comments
kanadgupta Apr 4, 2023
76ee1c7
refactor: safer deterministic way to indicate GHA args
kanadgupta Apr 4, 2023
440495b
chore: lint
kanadgupta Apr 4, 2023
ae006db
fix: use string-argv instead
kanadgupta Apr 5, 2023
073ac68
test: add another test case
kanadgupta Apr 5, 2023
054643f
test: remove redundant tests, clean up names
kanadgupta Apr 5, 2023
9fa7b48
chore: add some debug statements
kanadgupta Apr 5, 2023
d6c3f2f
revert: delete .DS_Store
kanadgupta Apr 5, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,18 @@ jobs:
with:
rdme: openapi:validate oas-examples-repo/3.1/json/petstore.json

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


# Docs: https://rdme-test.readme.io
- name: Run `openapi` command
uses: ./rdme-repo/
with:
rdme: openapi oas-examples-repo/3.1/json/petstore.json --key=${{ secrets.RDME_TEST_PROJECT_API_KEY }} --id=${{ secrets.RDME_TEST_PROJECT_API_SETTING }}

- name: Run `openapi` command with weird arg syntax
uses: ./rdme-repo/
with:
rdme: openapi "oas-examples-repo/3.1/json/petstore.json" --key "${{ secrets.RDME_TEST_PROJECT_API_KEY }}" --id=${{ secrets.RDME_TEST_PROJECT_API_SETTING }}
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ FROM alpine:3.14

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

ENTRYPOINT ["sh", "-c", "/exe/rdme $INPUT_RDME"]
ENTRYPOINT ["/exe/rdme"]
Copy link
Member Author

@kanadgupta kanadgupta Apr 4, 2023

Choose a reason for hiding this comment

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

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

CleanShot 2023-04-04 at 18 07 29@2x

Footnotes

  1. 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. โ†ฉ

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is better.

28 changes: 28 additions & 0 deletions __tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,34 @@ describe('cli', () => {
});
});

describe('args parsing in GitHub Actions runners', () => {
it('should return version from package.json for help command (standard argv)', () => {
return expect(cli(['help', '--version'])).resolves.toBe(pkgVersion);
});

it('should return version from package.json for help command (single string argv)', () => {
return expect(cli(['docker-gha', 'help --version'])).resolves.toBe(pkgVersion);
});

it('should validate file (standard argv)', () => {
return expect(
cli(['openapi:validate', '__tests__/__fixtures__/petstore-simple-weird-version.json'])
).resolves.toBe('__tests__/__fixtures__/petstore-simple-weird-version.json is a valid OpenAPI API definition!');
});

it('should validate file (single string with file path in quotes)', () => {
return expect(
cli(['docker-gha', 'openapi:validate "__tests__/__fixtures__/petstore-simple-weird-version.json"'])
).resolves.toBe('__tests__/__fixtures__/petstore-simple-weird-version.json is a valid OpenAPI API definition!');
});

it('should attempt to validate file (single string with file path with spaces)', () => {
return expect(cli(['docker-gha', 'openapi:validate "a non-existent directory/petstore.json"'])).rejects.toThrow(
"ENOENT: no such file or directory, open 'a non-existent directory/petstore.json"
);
});
});

describe('--help', () => {
it('should print help', () => {
return expect(cli(['--help'])).resolves.toContain('a utility for interacting with ReadMe');
Expand Down
3 changes: 3 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,6 @@ outputs:
runs:
using: docker
image: docker://ghcr.io/readmeio/rdme:8.6.0
args:
- docker-gha
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

- ${{ inputs.rdme }}
14 changes: 14 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
"prompts": "^2.4.2",
"semver": "^7.0.0",
"simple-git": "^3.13.0",
"string-argv": "^0.3.1",
"table": "^6.8.1",
"tmp-promise": "^3.0.2",
"update-notifier-cjs": "^5.1.5",
Expand Down
26 changes: 25 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import path from 'path';

import chalk from 'chalk';
import cliArgs from 'command-line-args';
import parseArgsStringToArgv from 'string-argv';

// We have to do this otherwise `require('config')` loads
// from the cwd where the user is running `rdme` which
Expand Down Expand Up @@ -33,7 +34,7 @@ import getCurrentConfig from './lib/getCurrentConfig';
* fake CLI calls.
* @return {Promise}
*/
export default function rdme(processArgv: NodeJS.Process['argv']) {
export default function rdme(rawProcessArgv: NodeJS.Process['argv']) {
const mainArgs = [
{ name: 'help', alias: 'h', type: Boolean, description: 'Display this usage guide' },
{
Expand All @@ -45,6 +46,29 @@ export default function rdme(processArgv: NodeJS.Process['argv']) {
{ name: 'command', type: String, defaultOption: true },
];

let processArgv = rawProcessArgv;

/**
* We have a weird edge case with our Docker image version of `rdme` where GitHub Actions
* will pass all of the `rdme` arguments as a single string with escaped quotes,
* as opposed to the usual array of strings that we typically expect with `process.argv`.
*
* For example, say the user sends us `rdme openapi "petstore.json"`.
* Instead of `process.argv` being this (i.e., when running the command via CLI):
* ['openapi', 'petstore.json']
*
* The GitHub Actions runner will send this to the `rdme` Docker image:
* ['openapi "petstore.json"']
*
* To distinguish these, we have a hidden `docker-gha` argument which we check for to indicate
* when arguments are coming from the GitHub Actions runner.
* 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') {
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ๐Ÿคž๐Ÿฝ

processArgv = parseArgsStringToArgv(rawProcessArgv[1]);
}

const argv = cliArgs(mainArgs, { partial: true, argv: processArgv });
const cmd = argv.command || false;

Expand Down