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

Export data model FirewallRuleProperties and INTERNET from @jupiterone/integration-sdk-core #315

Open
ndowmon opened this issue Aug 26, 2020 · 7 comments

Comments

@ndowmon
Copy link
Contributor

ndowmon commented Aug 26, 2020

As of v3.0.0, we export RelationshipClass from @jupiterone/integration-sdk-core for consistency. Projects that also reference FirewallRuleProperties and INTERNET from the data model still need the @jupiterone/data-model dependency until those get exported from the sdk.

@austinkelleher
Copy link
Contributor

Should we export those properties from the SDK in a sub-property? This type of import doesn't feel natural

import { INTERNET } from '@jupiterone/integration-sdk-core';

@aiwilliams
Copy link
Contributor

I agree, I really don't like that export. How about GlobalEntities.INTERNET, so import { GlobalEntities } from '@jupiterone/integration-sdk-core'?

@aiwilliams
Copy link
Contributor

FirewallRuleProperties has always bothered me because it's a TypeScript type for one kind of relationship. I'd prefer it were in a schema for a class of relationship, to carry forward the notion that we have schemas for graph objects. I wonder if we should copy this into the projects that use it for now, and get relationship schemas going.

@ndowmon
Copy link
Contributor Author

ndowmon commented Aug 26, 2020

I may contradict something I said just a few short days ago, but I am now thinking that this would be most appropriately handled by namespacing the data model as an export:

import { DataModel } from '@jupiterone/integration-sdk-core';

const someConst = DataModel.INTERNET;

It maybe still makes sense to import certain objects, such as RelationshipClass, directly, but namespacing would allow either

import { DataModel, RelationshipClass } from '@jupiterone/integration-sdk-core';

const HAS = RelationshipClass.HAS;

const USES = DataModel.RelationshipClass.USES;

I don't particularly like namespacing the INTERNET import inside GlobalEntities if that's not how it's currently imported from @jupiterone/data-model. Sorry @aiwilliams 🤷 😇

@austinkelleher
Copy link
Contributor

Fwiw, I'm not a fan of it being exported from the data-model like that either. I think it should have been under a sub-property like GlobalEntities inside of the data-model.

import { INTERNET } from '@jupiterone/data-model';

vs.

import { GlobalEntities } from '@jupiterone/data-model';

I think we should consider fixing this in the data-model package. Every change to the data-model is technically a breaking change since it's pre-1.0.0 right now.

@ndowmon
Copy link
Contributor Author

ndowmon commented Aug 26, 2020

@austinkelleher I agree that if anything, the way that INTERNET is exported should be fixed in @jupiterone/data-model. Opened ^ JupiterOne/data-model#37 to discuss in that project.

I'm still leaning towards exporting everything in data-model under the @jupiterone/integration-sdk-core.DataModel property.

@aiwilliams
Copy link
Contributor

I don't particularly like namespacing the INTERNET import inside GlobalEntities if that's not how it's currently imported from @jupiterone/data-model. Sorry @aiwilliams 🤷 😇

Oh, yeah, definitely would not do DataModel.GlobalEntities.INTERNET 👍

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

No branches or pull requests

3 participants