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

#486 Unify instantiation mechanisms #521

Merged
merged 19 commits into from
Dec 2, 2024

Conversation

willosborne
Copy link
Member

@willosborne willosborne commented Oct 25, 2024

Make nodes, relationships, metadata and interfaces all use the same code for instantiation

Addresses #486

@willosborne willosborne changed the title Unify instantiation mechanisms #486 Unify instantiation mechanisms Oct 25, 2024
Copy link
Member

@rocketstack-matt rocketstack-matt left a comment

Choose a reason for hiding this comment

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

Make nodes, relationships, metadata and interfaces all use the same code for instantiation

Addresses #486

Is #486 actually addressed in this PR? Interfaces seem to be generated exactly the same way, using the following pattern

{
  "$schema": "https://raw.githubusercontent.com/finos/architecture-as-code/main/calm/draft/2024-04/meta/core.json",
  "title": "Test CLI Pattern",
  "type": "object",
  "properties": {
    "nodes": {
      "type": "array",
      "minItems": 1,
      "prefixItems": [
        {
          "$ref": "https://raw.githubusercontent.com/finos/architecture-as-code/main/calm/draft/2024-04/meta/core.json#/defs/node",
          "type": "object",
          "properties": {
            "unique-id": {
              "const": "system-001"
            },
            "name": {
              "const": "Demo System"
            },
            "description": {
              "const": "Demo System to test generation"
            },
            "node-type": {
              "const": "system"
            },
            "interfaces": {
              "type": "array",
              "minItems": 1,
              "prefixItems": [
                {
                  "$ref": "https://raw.githubusercontent.com/finos/architecture-as-code/main/calm/draft/2024-04/meta/interface.json#/defs/rate-limit-interface",
                  "type": "object",
                  "properties": {
                    "unique-id": {
                      "const": "rate-limit-info"
                    }
                  }
                }
              ]
            }
          }
        }
      ]
    },
    "required": [
      "nodes"
    ]
  }
}

I get exactly the same instantiation whether generating with the 0.2.4 version of the CLI from npm or the locally built and installed version:

{
  "nodes": [
    {
      "unique-id": "system-001",
      "node-type": "system",
      "name": "Demo System",
      "description": "Demo System to test generation",
      "interfaces": [
        {
          "unique-id": "rate-limit-info",
          "key": "{{ REF_KEY }}",
          "time": -1,
          "time-unit": "{{ REF_TIME_UNIT }}",
          "calls": -1
        }
      ]
    }
  ],
  "relationships": [],
  "metadata": []
}

If the intent of this PR is purely refactor and not change the behaviour then seems correct.

We should probably also bump the patch version of the CLI in package.json and index.ts to 0.2.5 to ensure folks realise there was a change.

@willosborne
Copy link
Member Author

That's interesting, I have not seen the same behaviour. Let me try to reproduce, maybe there's something I've missed

@willosborne willosborne force-pushed the generic-instantiation branch 2 times, most recently from decc7b4 to 1d74ca8 Compare November 18, 2024 16:31
@willosborne willosborne force-pushed the generic-instantiation branch from 1d74ca8 to 609b63b Compare November 21, 2024 17:21
@willosborne
Copy link
Member Author

Alright, so some considerable more diving has revealed my problem.
The schema directory is smart enough to recursively resolve $refs when it's pulling out a definition.
But once you go to actually generate that definition later on, it's already thrown away the info about which schema those definitions belong to.

i.e. a $ref: #/defs/abcd within interface.json is indistinguishable from one that references a local def at the bottom of the current pattern.

So what I'm working on now is a means to disambiguate the local references (i.e. without a schema ID, beginning with just a #) by turning them into absolute references including the full URI.

Note that just using file references is probably going to break this for now. e.g. interface.json#/defs/rate-limit-key from another file won't get fully disambiguated to https://calm.com/pattern/etc

This is a little more gnarly than I thought; I'm wondering if maybe I should introduce a secondary data structure to track where things came from when they get generated.

@willosborne
Copy link
Member Author

Made some big improvements here. It now qualifies all relative references (at least, within the same file) with the full ID.
Also can do enums now too:
image
I also added a logging hint as to the potential values for an instantiated enum:
image

@willosborne willosborne requested a review from a team as a code owner November 24, 2024 17:42
it('qualify relative references within same file to absolute IDs', async () => {
const schemaDir = new SchemaDirectory();

await schemaDir.loadSchemas('../calm/draft/2024-04');
Copy link
Member

Choose a reason for hiding this comment

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

Potentially ignorable, but any reason why we aren't building tests to the most recent version on the schema?

Copy link
Member Author

Choose a reason for hiding this comment

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

No particular reason. Would you mind if I got this larger PR merged before going back and making these changes? It keeps getting broken by other changes 😆

@willosborne willosborne force-pushed the generic-instantiation branch from 5b5b37f to 560e6b6 Compare December 2, 2024 15:57
@rocketstack-matt rocketstack-matt merged commit a385540 into finos:main Dec 2, 2024
7 checks passed
rocketstack-matt added a commit that referenced this pull request Dec 3, 2024
* CLI Version Upgrade
Also fixing the CLI by;
Introducing `tsup` to bundle our packages together into a useable executable.
Migrated `spectral` into `shared` - updated README and steps - now the steps aren't referenced via file and are part of the code itself. Simplfying validation code.

* Cleaned up Spectral from CODEOWNERS

* Update dependency ts-graphviz to v2.1.5 (#629)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Missing package-lock.json (#631)

* Update docusaurus monorepo to v3.6.2 (#605)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Matthew Bain <[email protected]>

* Update dependency @types/node to v22.10.1 (#635)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update docusaurus monorepo to v3.6.3 (#633)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* This fixes #316 (#634)

Introducing a `-pretty` flag to the validate command.

* #486 Unify instantiation mechanisms (#521)

* add instantiate module and move relationships/metadata to use it

* #486 initial unification

* #486 add path context to generate command logging

* #486 fix condition when instantiating

* 486 clean up functions and split property instantiation

* #486 lint

* #486 always instantiate 'interfaces' block

* 486 test improvements

* #486 comments cleanup

* #486 test improvements

* Workspace Bin Movement

Now that I know more about workspaces, the published CLI didn't have a bin element so wasn't being linked as part of the release/install.

Also ran lint-fix - so some other line ending fixes.

* Instantiate refs and qualify relative paths when generating

* Extract update logic to higher-order function

* Add schema directory test for qualifying refs

* Properly instantiate enums (first cut)

* Fix typing issue and enum placeholder logic

* Log potential values for an enum when instantiated

* Suggested change to appendPath

* Lint

---------

Co-authored-by: Thels <[email protected]>

* #639 - Github Action Chore - Standardizing Existing - Introducing Docs Build for PRs (#643)

* chore: fixes #639

* Code review fixes

---------

Co-authored-by: Matthew Bain <[email protected]>

* Update docs url to new domain (#644)

* Update dependency globals to v15.13.0 (#641)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update typescript-eslint monorepo to v8.17.0 (#602)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

---------

Co-authored-by: Thels <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Matthew Bain <[email protected]>
Co-authored-by: Will Osborne <[email protected]>
Co-authored-by: David Johnston <[email protected]>
rocketstack-matt pushed a commit to rocketstack-matt/architecture-as-code that referenced this pull request Dec 5, 2024
* add instantiate module and move relationships/metadata to use it

* finos#486 initial unification

* finos#486 add path context to generate command logging

* finos#486 fix condition when instantiating

* 486 clean up functions and split property instantiation

* finos#486 lint

* finos#486 always instantiate 'interfaces' block

* 486 test improvements

* finos#486 comments cleanup

* finos#486 test improvements

* Workspace Bin Movement

Now that I know more about workspaces, the published CLI didn't have a bin element so wasn't being linked as part of the release/install.

Also ran lint-fix - so some other line ending fixes.

* Instantiate refs and qualify relative paths when generating

* Extract update logic to higher-order function

* Add schema directory test for qualifying refs

* Properly instantiate enums (first cut)

* Fix typing issue and enum placeholder logic

* Log potential values for an enum when instantiated

* Suggested change to appendPath

* Lint

---------

Co-authored-by: Thels <[email protected]>
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