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

adr for conceptual view #1190

Merged
merged 1 commit into from
Jan 27, 2025
Merged

adr for conceptual view #1190

merged 1 commit into from
Jan 27, 2025

Conversation

JimFuller-RedHat
Copy link
Collaborator

@JimFuller-RedHat JimFuller-RedHat commented Jan 23, 2025

@JimFuller-RedHat JimFuller-RedHat self-assigned this Jan 23, 2025
@JimFuller-RedHat JimFuller-RedHat force-pushed the adr-analysis-conceptual branch 16 times, most recently from c0bb407 to 7bc6b02 Compare January 23, 2025 12:58
docs/adrs/00002-analysis-graph.md Outdated Show resolved Hide resolved
docs/adrs/00002-analysis-graph.md Outdated Show resolved Hide resolved
@chirino
Copy link
Contributor

chirino commented Jan 23, 2025

I like it. We should consider if there are better terms to use than ancestor and descendant as these could get associated with the AncestorOf relationship. Maybe:

  • relationship - reverse-relationship
  • forward - reverse
  • normal and inverse

If the ancestor and descendant fields found in the response are optional then the same return type can be used for all these API endpoints. If that's the case then I think we could consolidate to a single api/v2/analysis/relationships endpoint if it accepts arg filters like:

  • ancestors bool
  • descendants bool
  • depth: int
  • realtionshipType: string

@JimFuller-RedHat
Copy link
Collaborator Author

JimFuller-RedHat commented Jan 23, 2025

I like it. We should consider if there are better terms to use than ancestor and descendant as these could get associated with the AncestorOf relationship. Maybe:

* `relationship` - `reverse-relationship`

* `forward` - `reverse`

* `normal` and `inverse`

In the graph world this conversation comes up regularly ... I always settle on anc/desc (for DAG) as most people agree what it means.

If the ancestor and descendant fields found in the response are optional then the same return type can be used for all these API endpoints. If that's the case then I think we could consolidate to a single api/v2/analysis/relationships endpoint if it accepts arg filters like:

* ancestors bool

* descendants bool

* depth: int

* realtionshipType: string

I am all for simplifying and/or having less 'moving parts' ... even better when parameterised ... in this case I suggested separation because it was easier to puzzle out ... lets discuss this in meeting.

@ctron
Copy link
Contributor

ctron commented Jan 24, 2025

I like it. We should consider if there are better terms to use than ancestor and descendant as these could get associated with the AncestorOf relationship. Maybe:

* `relationship` - `reverse-relationship`

* `forward` - `reverse`

* `normal` and `inverse`

In the graph world this conversation comes up regularly ... I always settle on anc/desc as most people agree what it means.

If the ancestor and descendant fields found in the response are optional then the same return type can be used for all these API endpoints. If that's the case then I think we could consolidate to a single api/v2/analysis/relationships endpoint if it accepts arg filters like:

* ancestors bool

* descendants bool

* depth: int

* realtionshipType: string

I am all for simplifying and/or having less 'moving parts' ... even better when parameterised ... in this case I suggested separation because it was easier to puzzle out ... lets discuss this in meeting.

Maybe, we can have both? A generalized endpoint, with the options described. And opinionated versions, which simply set defaults.

@JimFuller-RedHat JimFuller-RedHat force-pushed the adr-analysis-conceptual branch 3 times, most recently from 72312d4 to cc6aa77 Compare January 24, 2025 09:20
@ctron
Copy link
Contributor

ctron commented Jan 24, 2025

One idea, when it comes to relationships. Why not use the following serialization:

{
  "sbom_id": "",
  "node_id": "",
  "ancestors": [
    {
      "relationship": "AncestorOf",
      "node": {
         "sbom_id": "",
         "node_id": "",
         
      }
    }
  ]
}

Taking out the relationship of the actual node data.

IMO, this would separate the node from the relationship and might it a bit clearer when to expect which fields.

One alternative to this could be to have all relationships group in a map:

{
   "node_id": "",
   "ancestors": {
     "AncestorOf": [
       { "node_id": "anc1", }
       { "node_id": "anc2", }
      ]
   }
}

@JimFuller-RedHat
Copy link
Collaborator Author

One idea, when it comes to relationships. Why not use the following serialization:

I can see why this might be useful, though it also complexifies things - for what significant benefit ? if we can have a response payload that is amenable directly to visualisation I think that is the best goal to drive towards.

The biggest/last issue we need to define is conceptual view relationships - will try to get something proposed there before our meeting today.

@JimFuller-RedHat JimFuller-RedHat force-pushed the adr-analysis-conceptual branch 9 times, most recently from 327efec to c5eb868 Compare January 24, 2025 15:15
@JimFuller-RedHat
Copy link
Collaborator Author

@chirino, @ctron and @jcrossley3 please re review

@JimFuller-RedHat JimFuller-RedHat added this pull request to the merge queue Jan 27, 2025
Merged via the queue into main with commit b5fca5d Jan 27, 2025
2 of 3 checks passed
@JimFuller-RedHat JimFuller-RedHat deleted the adr-analysis-conceptual branch January 27, 2025 10:26
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