-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Update Hackuity integration: API key auth, Hackuity.Findings.Status.LastClosedAt #26558
Update Hackuity integration: API key auth, Hackuity.Findings.Status.LastClosedAt #26558
Conversation
Thank you for your contribution. Your generosity and caring are unrivaled! Make sure to register your contribution by filling the Contribution Registration form, so our content wizard @thefrieddan1 will know the proposed changes are ready to be reviewed. |
48d5fe9
to
e90e922
Compare
Summary of the checks in error below. This PR is now ready to review! The docker image tag is not the latest numeric tag, please update it.I did not change it since it was been reverted in #26405 [IN129] - You've removed integration parameters, the removed parameters are '{'login'}'This has been documented as a breaking change. [IN116] - You've added required, the field is 'apikey'This has been documented as a breaking change. |
e90e922
to
42aa714
Compare
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.
Hi @Rogdham,
thank you for your contribution we appreciate your effort, I reviewed the content in the PR and would like to provide some feedback:
Because the changes you introduce here are breaking changes I would recommend 2 possible options :
- Make the login and password hidden in the yaml file. ( do not remove them )
And keep the apikey changes with slight modifications. ( example for hidden apikey:
https://github.com/demisto/content/blob/master/Packs/AbuseDB/Integrations/AbuseDB/AbuseDB.yml - Or keep the breaking changes and add a note to the README file. Basically adding the breakingChangesNotes part from the json in the README as well. example :
https://github.com/demisto/content/blob/master/Packs/MicrosoftGraphSecurity/ReleaseNotes/2_1_21.md
The advantage of option 1 is that it does not break compatibility to users already using the integration. New users configuring the integration will not see the hidden user and password since its hidden and will configure it with the new apikey.
The advantage of option 2 is that its easier to add a note in the README.
But it might make existing users of the integration dealing the the breaking change.
So its up to you to decide how to proceed from here.
01894c2
to
e0cbeb1
Compare
Hello @thefrieddan1 and thank you for your review! We have chosen option 2, tell me if the wording change in the release notes looks better to you! |
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.
LGTM
30551d8
into
demisto:contrib/Rogdham_Hackuity_api_token_auth
For the Reviewer: Successfully created a pipeline in Gitlab with url: https://code.pan.run/xsoar/content/-/pipelines/5354508 |
* Update Hackuity integration: API key auth, Hackuity.Findings.Status.LastClosedAt (#26558) * Hackuity: API key auth, status last closed at * Update docker image. --------- Co-authored-by: Danny_Fried <[email protected]> * Bump version * align RN 1_0_7 with master * Deprecate login password and support api key * Add default value to deprecated fields. Update README * Take api key from hiddenuser name field. * Remove redundant method. * Remove redundant line. Make api key required. * Change display password. * Update Packs/Hackuity/Integrations/Hackuity/README.md Co-authored-by: Rogdham <[email protected]> * Remove new line in end of file in RN --------- Co-authored-by: Rogdham <[email protected]>
Status
Description
This PR makes the following changes:
Hackuity.Findings.Status.LastClosedAt
Minimum version of Cortex XSOAR
Does it break backward compatibility?
Must have