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

feat: simplify the input interface #4

Merged
merged 8 commits into from
Jul 14, 2023
Merged

feat: simplify the input interface #4

merged 8 commits into from
Jul 14, 2023

Conversation

kettanaito
Copy link
Contributor

@kettanaito kettanaito commented Jul 13, 2023

This pull request reflects on a larger discussion about the GitHub Action.

Changes

  • Removes all granular action inputs. Instead, a single command input should be used.
  • Removes the report action. I'd not have any implicit behaviors, especially when they deviate from the raw CLI experience. The users can use the --output flag to generate the report and access it in the file system since they know the path already.
  • Removes usage examples from README.md. Directs to the appropriate page in the docs. We shouldn't duplicate these examples, let them live in a single place. Easy to find, easy to manage.
  • Adjusts the test workflow.
  • Adds a new test to ensure that the generated report can be accessed by other steps of the job (e.g. to manipulate it, upload it as an artifact, etc).

@kettanaito kettanaito marked this pull request as ready for review July 13, 2023 15:15
README.md Outdated Show resolved Hide resolved
entrypoint.sh Outdated Show resolved Hide resolved
entrypoint.sh Outdated
${DOTENV:+--dotenv $DOTENV}
${INSECURE:+--insecure}
${QUIET:+-q}
/home/node/artillery/bin/run "./artillery.report.yml" $1

Choose a reason for hiding this comment

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

if I understand this correctly, you're trying to set output to always be the same hardcoded value, right? Aren't you missing the --output flag then?

Also the artillery report default format is .json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about this and decided that it's not a good idea to introduce this implicit behavior for the action alone. The regular run command doesn't produce any implicit reports, neither should the action. Let the usage be identical, no matter if you're using an action or tapping into the CLI directly.

Choose a reason for hiding this comment

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

How would a user extract a report from the Github Action though? We might want to showcase this in an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by "extract"?

One of the ways they may want to operate with the generated report is by uploading it as an artifact on the workflow to observe after the run.

- uses: artilleryio/action-cli@v1
  with:
    command: run test.yml --output report.json

- uses: actions/upload-artifact@v3
  with:
    name: artillery-report
    path: report.json

I believe we have this example in the docs. If we don't, I will add it once we publish the action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One good point from this is we should add an automated test to ensure the report written in the Docker image is actually available outside of the step 💯 Will add that.

Copy link
Contributor Author

@kettanaito kettanaito Jul 14, 2023

Choose a reason for hiding this comment

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

Added a basic test that the generated report exists in the file system. Waiting for it to pass...

Edit: Confirm that the generated report is accessible further into the job! ✅

Copy link
Member

@hassy hassy left a comment

Choose a reason for hiding this comment

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

:shipit:

@kettanaito kettanaito merged commit 84fad5b into main Jul 14, 2023
2 checks passed
@kettanaito kettanaito deleted the feat/simplify branch July 14, 2023 11:31
@kettanaito kettanaito mentioned this pull request Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants