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

Scan report: Duplicate issue creation when artifactName passed #20

Closed
tony opened this issue Dec 3, 2021 · 10 comments · Fixed by #60
Closed

Scan report: Duplicate issue creation when artifactName passed #20

tony opened this issue Dec 3, 2021 · 10 comments · Fixed by #60
Labels
bug Something isn't working

Comments

@tony
Copy link
Contributor

tony commented Dec 3, 2021

image

Since readPreviousReport isn't checking for the artifact name, it will create a new issue when artifact_name is passed in through a PR like zaproxy/action-baseline#64

@tony
Copy link
Contributor Author

tony commented Feb 22, 2022

Closing this as I don't recall the original issue as too much time has passed. If anyone bumps into this in a future feel free to reopen and cite this. Thank you!

@thc202
Copy link
Member

thc202 commented Mar 10, 2023

@dbartholomae do you want to take a look at this one? There are still places where zap_scan is used but should use the artifactName instead. I guess we should also use artifactName for the upload without issue creation too.

@dbartholomae
Copy link
Contributor

I can look into this, but would like to first make sure that we have a full release cycle running including the GitHub actions, as any change that we currently do otherwise might not yet actually reach any users :)

@thc202
Copy link
Member

thc202 commented Mar 19, 2023

But this introduces a bug, that's why it would be better to address it before the release. (Well, we could always not expose the artifact name in the actions yet.)

@dbartholomae
Copy link
Contributor

Oh, I thought that this was a bug that is already in the released code. In that case, I'll look into it before releasing.

@dbartholomae
Copy link
Contributor

Are you sure that this introduces a bug? I don't see how the code currently in production is different here from the code that we would release.

@tony
Copy link
Contributor Author

tony commented Apr 2, 2023

I am currently on a diverged fork of these packages and don't have the time to investigate this for now - as I over a year has passed.

@dbartholomae @thc202 What do you propose I do? I can close the issue. I do not have to investigate this again in the short term since I'd have to bootstrap, see if it's happening with the latest release, and familiarize myself wit hthe code again

@dbartholomae
Copy link
Contributor

dbartholomae commented Apr 2, 2023

@tony I've started last year to modernize the package by adding TypeScript, tests, and linting, with the idea to add some features like grouping alert messages by severity in the issues that are created (see #48)). The next step from my point of view would be to release the code currently in this repo to be used in the actions so that we have a simple release flow. The reason why I commented in this issue is that if I understood @thc202 correctly, they believe that this is a bug that needs to be fixed before we can use the code in this repo for GitHub actions as it would otherwise add a new bug.
My current understanding is that this is an existing bug and should not block releasing the current code.

thc202 added a commit to thc202/actions-common that referenced this issue Apr 2, 2023
Use custom artifact name when reading the previous report not always
using the old/hardcoded name otherwise it would miss the artifact and
lead to new issues always.
Also, upload the artifact with the custom name even if there's no issue
raised as following runs my start raising it.

Close zaproxy#20.
See also zaproxy#21, which is the same but before the refactoring to
TypeScript.

Signed-off-by: thc202 <[email protected]>
thc202 added a commit to thc202/actions-common that referenced this issue Apr 2, 2023
Use custom artifact name when reading the previous report not always
using the old/hardcoded name otherwise it would miss the artifact and
lead to new issues always.
Also, upload the artifact with the custom name even if not raising
issues as following runs my start raising them.

Close zaproxy#20.
See also zaproxy#21, which is the same but before the refactoring to
TypeScript.

Signed-off-by: thc202 <[email protected]>
@thc202
Copy link
Member

thc202 commented Apr 2, 2023

Raised a PR (#60), please take a look.

@thc202
Copy link
Member

thc202 commented Apr 2, 2023

#20 (comment)

Yes, the custom artifact name was not yet released. As mentioned before (I think), it's true that if we don't use the new feature the bug would not be a problem but IMO better release without the issue in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
4 participants