-
Notifications
You must be signed in to change notification settings - Fork 4
INT-4833 - Add Consumable, License and User #30
INT-4833 - Add Consumable, License and User #30
Conversation
Socket Security Pull Request Report👍 No new dependency issues detected in pull request Pull request report summary
Bot CommandsTo ignore an alert, reply with a comment starting with Ignoring: Powered by socket.dev |
Converting to a draft to 1) investigate the socket warning and 2) test the socket slack alert when it is moved back to a PR. |
Researched socket alert https://jupiterone.atlassian.net/browse/DESK-1995 |
@SocketSecurity ignore [email protected] |
I think recordings are not working because your |
Thanks for looking into this! It turns out we had to re-record every recording/test anyway, so I've set the context.ts URL to localhost:8000. The other option perhaps would have been to modify recording.har(s) and change localhost:8000 to the URL we had in the context.ts, but considering there isn't a mixture of both this feels more proper? Also, I heard we don't have a partner account so any additional dev efforts might use the (free) stand-alone version again (meaning localhost). |
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.
in general looks good! Just a few small comments.
@@ -8,6 +8,33 @@ and this project adheres to | |||
|
|||
## [Unreleased] | |||
|
|||
- Reorganized files |
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.
This changelog doesn't reflect what I'm seeing in jupiterone.md
. I think it needs to be updated
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.
Hey Max, I think you might be referring to the mapped relationships. The thing is, previous version wasn't using mappedRelationships
array to declare them in the step functions so they were put next to the regular ones. So we aren't creating them thanks to some code change now, but we just fixed that part so that it's properly displayed in the jupiterone.md
now. I'm not sure if we should document that in the CHANGELOG, should we?
Perhaps you've meant something different entirely 🤔
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.
ah I see that makes sense. Thanks for the explanation!
package.json
Outdated
"@jupiterone/integration-sdk-core": "^5.5.0", | ||
"@jupiterone/integration-sdk-dev-tools": "^5.5.0", | ||
"@jupiterone/integration-sdk-testing": "^5.5.0", | ||
"@jupiterone/integration-sdk-core": "^7.4.0", |
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.
any reason we didn't jump to latest version?
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.
No particular reason, I've updated it now, thanks for the reminder! It did complain that hardware converter needs deviceId
property to be set now and I've used the id that gets sent along with hardware response.
@@ -21,6 +24,7 @@ export async function fetchAccount({ | |||
|
|||
const accountEntity = getAccountEntity(instance); | |||
await jobState.addEntity(accountEntity); | |||
await jobState.setData(ACCOUNT_ENTITY_KEY, accountEntity); |
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.
if you know the account entity key or are able to derive at a future time, you can avoid setting this data in the jobState and instead use jobState.findEntity
when you need this entity in future steps.
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.
This is interesting, I think previously we've always used setData
/getData
to store those "singleton" entities such as Service
(e.g. scanner) and more importantly Account
.
Is this a new idea for findEntity
to be used instead? Using setData
/getData
felt like pattern and we've got into to the habit of using it.
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 when findEntity was introduced, but it is what I've personally gone with to avoid storing redundant copies of entity data we already have. All that being said for single entities like Service, Account etc this really isn't a big concern. Just pointing it out as an alternative option. I wouldn't hold up a PR over this :)
jobState, | ||
}: IntegrationStepExecutionContext<IntegrationConfig>) { | ||
const client = createServicesClient(instance); | ||
const accountEntity = (await jobState.getData(ACCOUNT_ENTITY_KEY)) as Entity; |
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.
could use jobState.findEntity
if you know account entity key. If you don't you can store it with jobState.setData
instead of the entire entity object. More space efficient.
|
||
consumableUsers.map(async (consumableUser) => { | ||
if ((consumableUser as ConsumableUser).name) { | ||
const $ = cheerio.load((consumableUser as ConsumableUser).name); |
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.
Maybe a small comment here about what cheerio is and what it is used for. I see it is for jquery for server-side, but not obvious unless I google.
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.
The list of users that use a consumable is returned as a link to those users. We used cheerio to easily manipulate the response. I've added a comment to detail this, as suggested.
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.
Thanks for the work on this!
🚀 PR was released in |
Description
Thank you for contributing to a JupiterOne integration!
Please include a summary of the change and which issue is fixed. Please also
include relevant motivation and context. List any dependencies that are required
for this change.
Summary
Entities
_type
_class
snipeit_consumable_resource
Resource
snipeit_licensed_application
Application
snipeit_user
User
Relationships
_type
_class
_type
snipeit_account
snipeit_consumable_resource
snipeit_account
snipeit_licensed_application
snipeit_account
snipeit_user
snipeit_user
snipeit_consumable_resource
Mapped Relationships
_type
_class
_type
snipeit_user
*person*
Type of change
Please leave any irrelevant options unchecked.
not work as expected)
Checklist
General Development Checklist:
Integration Development Checklist:
Please leave any irrelevant options unchecked.
endpoints, and have documented any additional permissions in
jupiterone.md
, where necessary.API
using
dependsOn
JupiterOne data model
to ensure that any new entities/relationships, and relevant properties,
match the recommended model for this class of data
CHANGELOG.md
file to describe my changesreviewed all existing managed questions referencing the entities,
relationships, and their property names, to ensure those questions still
function with my changes.