[BUG] Cyclic Versioning of phenix/types/versions/v*

[nblair2]

Bug Report

Description

Currently, the phenix/types/version/v1 package imports phenix/types/version/v2, which means v2 cannot import anything from v1 without creating an import cycle.

Additional Context

I was adding a new TopologySpec to v2 and wanted to reuse the existing v1 Node struct, but that introduces a cyclic import. A few paths forward:

• Create an internal domain model and use adapters between each versioned spec and the domain model. More work up front, but clean and future‑proof.
• Keep versions consistent across all types (introduce v2 equivalents for any reused v1 types) to avoid cross‑version imports. Easier short term, but leads to duplication.
• Implement the changes in v1 topology if they are backward‑compatible, deferring a proper refactor.

Checklist

• I have included no proprietary/sensitive information in

[activeshadow]

@nblair2 I'm curious what in v2 is being imported into v1 (I'll have to go look). A proper design would be that v1 should know nothing about v2 and thus shouldn't import it. Since v2 should know about v1 it would be okay for v2 to import v1.

As for an internal domain model, the interfaces should be providing that.

[activeshadow]

By the way, I'm 100% positive I'm the one that imported v2 into v1. I'm thus the one that didn't implement this properly.

I was likely trying to avoid requiring an upgrade to the experiment spec version in yaml configs when I upgraded the scenario versioning (a long time ago).

I should have never put a forward looking import into a v1 package, so I likely made this harder to fix now (existing experiments will have to be updated to be v2 if we remove the v2 reference and create a new v2 experiment spec). We do have existing code to provide an auto upgrade path for specs, we'd just have to see if that approach would work for experiments (I think it would...).

[activeshadow]

@nblair2 what are the topo changes? Is there a chance they're backwards compatible? Regardless, I guess we will need to address this issue at some point so now is as good a time as any!

[nblair2]

@activeshadow v2.ScenarioSpec is being imported into v1.ExperimentSpec. I can see how it happened, scenario went v1->v2 before experiment when v0->v1.

Based on that pattern of imports, just changing the organization from phenix/types/version/v1/scenario.go (grouped by version number) to phenix/types/version/scenario/v1.go (grouped by type) might work - to keep things independent of version and allow experiment versions to grow at a different rate than scenario versions, for example.

That all being said, I think the easy way out that I am going to take is just jam my changes into the existing v1.Topology, since they are backwards compatible. My change is adding the ability for topos to pull in other topos (like the sceanrio -fromScenario: field. Something like this:

    Topology:
      type: object
      properties:
        includeTopologies:
          type: array
          nullable: true
          items:
            type: string
          example:
          - IT-topo.yml
          - OT-topo.yml
        nodes:
          type: array
          items:
            oneOf:
            - $ref: '#/components/schemas/minimega_node'
            - $ref: '#/components/schemas/external_node'

[GhostofGoes]

@activeshadow v2.ScenarioSpec is being imported into v1.ExperimentSpec. I can see how it happened, scenario went v1->v2 before experiment when v0->v1.

Based on that pattern of imports, just changing the organization from phenix/types/version/v1/scenario.go (grouped by version number) to phenix/types/version/scenario/v1.go (grouped by type) might work - to keep things independent of version and allow experiment versions to grow at a different rate than scenario versions, for example.

That all being said, I think the easy way out that I am going to take is just jam my changes into the existing v1.Topology, since they are backwards compatible. My change is adding the ability for topos to pull in other topos (like the sceanrio -fromScenario: field. Something like this:

Topology:
  type: object
  properties:
    includeTopologies:
      type: array
      nullable: true
      items:
        type: string
      example:
      - IT-topo.yml
      - OT-topo.yml
    nodes:
      type: array
      items:
        oneOf:
        - $ref: '#/components/schemas/minimega_node'
        - $ref: '#/components/schemas/external_node'

Unrelated to the issue at hand, but maybe fromTopologies would be better for consistency with fromScenario?

[nblair2]

@GhostofGoes agree.

I shied away from fromScenario analog because it constrains to only importing one app at a time. Not a problem for scenario apps, because they are long and few, but I don't want to only import one node at a time.

Happy to use the same fromTopology nomenclature, but it will be slightly different.

[activeshadow]

Based on that pattern of imports, just changing the organization from phenix/types/version/v1/scenario.go (grouped by version number) to phenix/types/version/scenario/v1.go (grouped by type) might work - to keep things independent of version and allow experiment versions to grow at a different rate than scenario versions, for example.

This is definitely a good idea worth looking into to see if there would be any circular dependency requirements with this type of organization. Would also need to see how much of a refactor this would burden the rest of the code base with.