-
Notifications
You must be signed in to change notification settings - Fork 18
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: ignore-on-exit flag to control the error code #162
Conversation
@baruchiro is the "--fail-on" a typo and should be "--ignore-on-exit"? In addition, would you like just the name of the argument and the necessary behavioral changes to happen (from bool to string)? or should I look more in depth in the kics code for a different way? Current implementation:
Ran manual tests with the confluence plugin: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I finished my review, but there are enough comments for now.
I think you copied the Kics code, but you didn't adjust it to our code.
Coping code is OK! I believe in this. But when I copy the code, I read it and learn it and make sure it is good at least as code I would write myself.
Background info about Kics workfolow:
in case of a non-nil error, the One exception happenes when a scan results with a non-zero code. In that case, the program will exit using an explicit 2ms Proposed Changes:
There are at least two exceptions that I spotted:
Further thoughts:In kics there is a seperate package "helpers" to determine the exit code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
It was not as easy as I thought, sorry, but you're doing good work!
Maybe I have more comments, but I'm sending it now to not delay you.
Hi, I tried to review your PR and I saw the I know it will conflict your PR, but I think your work can be better and easier now. Please let me know if you need help with the conflict or something else. Please do not get confused and re-create another PR to skip the conflict. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay
Proposed Changes:
Next to do:
|
feat: confluence url is mandatory arg (Checkmarx#160)
@nirmo Please let me know when you're ready for the next review |
0b7510b
to
7cc6483
Compare
Fixes #155
Reference:
ignore-on-exit
in KicsWaits for #188
Proposed Changes
2ms confluence --url <url> --spaces <space> --token <token> --username <>
2ms confluence --url <url> --spaces <space> --token <token> --username <> --ignore-secrets