Skip to content

fix!: replace deprecated assets API#251

Merged
branchv merged 16 commits intomainfrom
nionic-updates-lite
Mar 23, 2022
Merged

fix!: replace deprecated assets API#251
branchv merged 16 commits intomainfrom
nionic-updates-lite

Conversation

@TrippWhaley
Copy link
Copy Markdown
Contributor

Why?

What changed?

  • Remove Assets Service/Command/Models
  • Remove mention of asset_id in other models
  • Remove methods requiring asset_id
  • Create Nionic Service
  • Use Nionic as source of truth for facilities commands
  • Add Metrics Data and Labels get capabilities

Comment thread contxt/services/base_graph_service.py Outdated
),
)

org_id_slugs = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to embed the org slug in the JWT token that gives us the org_id? Having this mapping hardcoded is publicly listing our customers which can run afoul of service agreements.

Copy link
Copy Markdown
Contributor Author

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 think a better long-term plan would be to have some kind of service-discovery in place. As for this, I was just going with what we did in the js sdk already

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In reality, we should be moving away from orgId in my mind. It will be a stale concept once all services are single tenant. Instead, you would access everything my tenant slug. This is fine for now, we will just have to update later one. Could we potentially allow passing either orgId or slug and just take the slug as-is?

Copy link
Copy Markdown
Contributor

@luther-at-ndustrial luther-at-ndustrial left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! I love the metric cli additions!

@TrippWhaley TrippWhaley marked this pull request as ready for review March 17, 2022 19:03
ApiField("created_at", data_type=Parsers.datetime),
ApiField("weather_location_id"),
ApiField("slug"),
from sgqlc.types import Arg, Enum, Field, Input, Type, list_of, non_null
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this generated or manually written?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken from the codegen output but I was having some trouble getting it working here, so manually written with a template

Comment thread contxt/services/base_graph_service.py Outdated
),
)

org_id_slugs = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In reality, we should be moving away from orgId in my mind. It will be a stale concept once all services are single tenant. Instead, you would access everything my tenant slug. This is fine for now, we will just have to update later one. Could we potentially allow passing either orgId or slug and just take the slug as-is?

@TrippWhaley
Copy link
Copy Markdown
Contributor Author

@mgagliardo91 @luther-at-ndustrial I made an option to pass in either a slug or an organization id which may be helpful, but only really for library usage for internal services/cli usage for users with more than one org_id in their token. Not sure if this is preferred, I can revert the commit if it feels messy or unhelpful

Comment thread contxt/services/api.py
super().__init__(env, auth, **kwargs)
self.endpoint = RequestsEndpoint(f"{self.base_url}/graphql", session=self.session)

def query(self, query: str, variables: Optional[Any] = None) -> Dict:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@branchv branchv force-pushed the nionic-updates-lite branch from 793c33b to 18d945a Compare March 21, 2022 19:44
@branchv branchv changed the title Nionic Updates l fix!: replace deprecated assets API Mar 21, 2022
@branchv branchv merged commit d734070 into main Mar 23, 2022
@branchv branchv deleted the nionic-updates-lite branch March 23, 2022 12:30
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

Successfully merging this pull request may close these issues.

4 participants