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

Add artifact name as argument but enforce a positional default #15

Merged
merged 1 commit into from
Oct 14, 2021

Conversation

AlexDCraig
Copy link
Contributor

@AlexDCraig AlexDCraig commented Oct 6, 2021

The purpose of this PR is to allow for downstream dependencies to have custom report/artifact names. The default argument is to make sure packages dependent on this library that I am not aware of do not break from the introduction of this small feature.

Signed-off-by: AlexDHoffer <alexdchoffer@gmail.com>
@thc202
Copy link
Member

thc202 commented Oct 14, 2021

Thank you!

@kingthorin
Copy link
Member

Good to merge?

@thc202
Copy link
Member

thc202 commented Oct 14, 2021

I think so, the changes look ok to me.

@kingthorin
Copy link
Member

Just wasn’t sure if it needed to be coordinated with anything else.

@kingthorin kingthorin merged commit dc737b2 into zaproxy:master Oct 14, 2021
@thc202
Copy link
Member

thc202 commented Oct 14, 2021

Changes in this lib are fine to merge anytime.

@tony tony mentioned this pull request Dec 3, 2021
@tony
Copy link
Contributor

tony commented Dec 3, 2021

For later (this would be breaking and I don't want to delay a new release):

In a future version we may want to move artifactName to a new options object (similar to @actions/artifact's params) to make params more readable in the future

e.g. actionHelper.uploadArtifacts(workSpace, mdReportName, jsonReportName, htmlReportName, {artifactName})

That way options will be more easy to manage and we won't have the dreaded case where an additional position argument forces future downstream users pass undefined to fill 'zap_scan': actionHelper.uploadArtifacts(workSpace, mdReportName, jsonReportName, htmlReportName, undefined, newParam)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants