Registry API: Supporting artifacts on resource revisions #774

shrutiparabgoogle · 2022-10-06T19:19:54Z

This discussion started when we were thinking about supporting a history of Scores. As we are building more UI features, it is getting more and more difficult to keep track of which spec revision a particular artifact (conformance report, Score, scorecard) belongs to. This is a limitation in conveying the correct information.

Can we evaluate the option of attaching artifacts to a resource's revision?

Motivation:

In our case, we will attach artifacts to SpecRevisions and DeploymentRevisions and keep Apis, Versions, Project as is (resources with no revisions).
This goes in line with our data model, the artifacts that get generated for specs are usually properties of a particular revision (lint reports, scores, etc), same with deployments (security analysis of a particular deployed state). The changes to these artifacts are also usually governed by changes in the revision.
For other generic properties, like Related links, Source code repo, those are usually higher level properties outside the scope of a spec/deployment which will normally get attached to an API. For an API, spec becomes the implementation detail of that API (and deployment becomes an operational detail), hence we predict that most of the characteristics related artifacts will be attached at the API level since that is the unit that users are managing in the registry.
Such artifacts at API level will not have to worry about revisions, they will be one time artifacts which will be preserved whereas for Specs and Deployments, artifacts will need a regeneration every time a new revision gets created.
Following AIP-162, the implementation will look like this:

The example is for Specs here, deployments will also have a similar behavior:

Create/Update: Users should create artifacts only under a SpecRevision.
- The artifact path will look like this:
  projects/demo/locations/global/apis/petstore/versions/v1/specs/openapi.yaml@0000/artifacts/conformance-report
- The parent of the created artifact must always include the revision ID of the spec.
- When a user creates an artifact without specifying the revision ID, it should always get attached to the latest revision.
- The response from the API will always include the revision ID in the artifact path.
List/Read: Users should be able to list artifacts under SpecRevision and for a particular Spec
- A list request like projects/demo/locations/global/versions/v1/specs/openapi.yaml@0000/artifacts/- should list all artifacts
  under the specified revision.
- A list request without the revision ID, projects/demo/locations/global/versions/v1/specs/openapi.yaml/artifacts/- should list
  artifacts under the latest revision of that spec.
- A list request with wildcard, projects/demo/locations/global/versions/v1/specs/openapi.yaml@-/artifacts/- should list all artifacts under all revisions of that spec.
- The response from the API will always include the revision ID in the artifact path.

The above implementation is inline with the second scenario explained here

Some caveats to call out:

Whenever a user attaches an artifact to a spec without specifying the revision, it will be attached to the latest revision, that might have been updated by someone else and might not be the same as the user had created before.
Users can no longer create artifacts under a spec, they will always be attached to a specific revision.
Users should be aware that when they create a new revision of spec, the list call projects/demo/locations/global/versions/v1/specs/openapi.yaml/artifacts/- will no longer return artifacts that were available on the previous revision, they will have to create new artifacts on the latest revision for the list call to return artifacts.

The text was updated successfully, but these errors were encountered:

theganyo · 2022-10-10T17:48:00Z

I assume this applies higher in the path hierarchy as well? So taking the above heuristic, I might expect that this path:

projects/demo/locations/global/versions/v1/specs/-/artifacts/-

would obey the above rule: "A list request without the revision ID, projects/demo/locations/global/versions/v1/specs/openapi.yaml/artifacts/- should list artifacts under the latest revision of that spec."

I believe that implies that that request must list only the artifacts under the latest revision of every spec?

Whereas, to list the all artifacts of all the revisions, one would request this path:

projects/demo/locations/global/versions/v1/specs/-@-/artifacts/-

Is this correct? @shrutiparabgoogle @timburks

shrutiparabgoogle · 2022-10-10T17:53:07Z

Yes, that sounds correct @theganyo

timburks · 2022-10-10T19:51:42Z

I agree - that seems like a good interpretation.

theganyo · 2022-10-12T23:40:03Z

Given this change, I wonder... What was the reason for considering Spec and SpecRevision as separate entities? Would merging these into a Spec (with an optional revision field) be a reasonable simplification? Or would this potentially cause more issues than it solves?

theganyo · 2022-10-13T23:26:20Z

Discussion on merging Spec and SpecRevision here: #782

theganyo · 2022-10-19T22:02:50Z

(Note: Scrapping the above idea of changing naming entities.)

Question related to SpecRevision and DeploymentRevision: Are these entities something that the controller needs to support explicitly? For example, listResources can now return artifacts who's parents are SpecRevisions:

registry/cmd/registry/controller/controller.go

Lines 192 to 201 in 3aacfd1

 // Generate resource list 

 resourceList, err := listResources(ctx, client, resourcePattern, filter) 

 if err != nil { 

 return nil, nil, err 

 } 

 // Iterate over a list of existing target resources to generate update actions 

 for _, targetResource := range resourceList { 

 visited[targetResource.ResourceName().ParentName().String()] = true

This won't work properly with the controller as it exists as it expects to work with Specs. As such, we could either add support SpecRevisions as targets or translate the SpecRevisions to Specs. Thoughts, @shrutiparabgoogle ?

theganyo · 2022-10-20T18:45:17Z

Maybe I can clarify a bit with an example: Basically, I'm trying to clarify a test such as this: https://github.com/apigee/registry/blob/main/cmd/registry/scoring/score_test.go#L200. We've said that we should transparently assume the latest SpecRevision for a Spec. So, in this test, it's all operating at the Spec level. It gets artifacts (retrieved from the latest Spec revision) which are scored and a related artifact is set (on the latest Spec revision). So is this test (basically) still correct? And is the scoring as it exists correct - where it ignores the actual SpecRevision on both for the input and output side when processing Artifacts? Or do we need the scoring resolve a specific SpecRevision for its artifacts after processing the inputs and maintain that through to the output?

theganyo · 2022-10-20T20:23:53Z

Ok, I discussed a bit with @shrutiparabgoogle and I'm going to try to make the controller's patterns.SpecName resource bridge the gap between the names package names.Spec and names.SpecRevision.

And likewise, for Deployment and DeploymentRevision. This should allow the controller to continue to work like the API (which doesn't differentiate between Spec and SpecRevision or Deployment and DeploymentRevision). Then, if we want to refactor the names package to better address this impedance, we can do that later.

shrutiparabgoogle · 2022-10-20T20:27:05Z

Sounds good. The controller currently doesn't have support for deployments. Once we figure out the SpecRevisions, we can implement the Deployments in the same way.

shrutiparabgoogle · 2022-10-26T19:04:21Z

I think it's high time I add some detailed explanation on how the controller works currently and what is expected out of it after the revisions change:

Take an example of a sample manifest here:

- pattern: apis/-/versions/-/specs/-/artifacts/lintstats-gnostic
   dependencies:
   - pattern: $resource.spec/artifacts/lint-gnostic
   - pattern: $resource.spec/artifacts/complexity
   action: "registry compute lintstats $resource.spec --linter gnostic"

The controller will process the above manifest in two different scenarios:

The target resource needs an update

Current behavior:

The controller will fetch the spec level artifact lintstats-gnostic
Check if dependencies exist, in this case spec level artifacts lint-gnostic, complexity.
Compare the timestamp of all the above 3 artifacts, and determine if an update is required.
If yes, execute the action by providing it the spec.

In this current behavior, we have no way to figure out which revision was used to compute all of the above mentioned artifacts, hence we need a connection of revisionIDs in all the specs that we are using.

Expected behavior with revisions:

The controller will fetch the revision level artifact: The list call can be the same: apis/-/versions/-/specs/-/artifacts/lintstats-gnostic, this will fetch the artifacts on the latest spec revision and the artifact will look something like this:
projects/demo/locations/global/apis/petstore/versions/v1/specs/openapi.yaml@12345/artifacts/lintstats-gnostic.
Check if dependencies exist, fetch revision level artifacts lint-gnostic, complexity of the specific revision openapi.yaml@12345.
Compare the timestamp of all the above 3 artifacts, and determine if an update is required.
If yes, execute the action by providing it the revision: registry compute lintstats openapi.yaml@12345

Maintaining a specific revision throughout will ensure that there is no discrepancy, if the latest revision changes by the time the controller comes to executing the actions, we again face the same problem that we have in the current behavior. Hence let's not rely on the latest revision, latest revision should only be used while fetching the first set of artifacts of target resource, after that everything else should explicitly include the revisionID.

The target resource doesn't exist yet, needs creation

Current behavior:

The controller will fetch the list of non-visited parents, from this list of specs apis/-/versions/-/specs/-
For every parent in the list, figure out which target resources are expected to be created. In this case, figure out for which specs should we create lintstats-gnostic artifacts.
Check if dependencies exist, fetch spec level artifacts lint-gnostic, complexity.
If they exist, execute action by providing the spec.

Expected behavior with revisions:

The controller will fetch the list of non-visited parents, from this list of specs apis/-/versions/-/specs/-.
This step will need the most modifications:
- to fetch the non-visited parents, ignore the revisionID of the visited parents. For example, if the set of visited parents includes ["projects/demo/locations/global/apis/petstore/versions/v1/specs/openapi.yaml@12345", "projects/demo/locations/global/apis/apigeeregistry/versions/v1/specs/openapi.yaml@abcdef"], then the query should be such that it ignores the revisionIDs. List specs query should look like
```
list: "apis/-/versions/-/specs/-" 
filter: "exclude [projects/demo/locations/global/apis/petstore/versions/v1/specs/openapi.yaml,  projects/demo/locations/global/apis/apigeeregistry/versions/v1/specs/openapi.yaml] "   #the filter will be translated into a CEL expression. 
```
- The returned list of specs should also include their latest revisionIDs. This will require some modification to the listResources function to handle the Specs case, append latest revisionID while returning the SpecName
  The response from the listResources call for Specs should look like this:
```
projects/demo/locations/global/apis/apigee/versions/v1/specs/openapi.yaml@09876
projects/demo/locations/global/apis/translate/versions/v1alpha1/specs/openapi.yaml@vwxyz
```
For every parent in the list, figure out which target resources are expected to be created. In this case, figure out for which specs revisions should we create lintstats-gnostic artifacts for. We will need to generate the following two artifacts:
- projects/demo/locations/global/apis/apigee/versions/v1/specs/openapi.yaml@09876/artifacts/lintstats-gnostic
- projects/demo/locations/global/apis/translate/versions/v1alpha1/specs/openapi.yaml@vwxyz/artifacts/lintstats-gnostic

For each identified resource above,

Check if dependencies exist, fetch revision level artifacts lint-gnostic, complexity.
If they exist, execute action by providing the specRevision.

@theganyo @timburks Let me know if this sounds okay. The explanations mentioned above don't require any change in the definition of the controller's manifest, thus no user facing change. If we decide to separate out Spec and SpecRevisions and provide support for both (in the future), that will be a new feature in the controller.

theganyo · 2022-10-26T20:19:24Z

Thanks for the clarification. This makes sense to me.

theganyo · 2022-10-27T20:29:49Z

One issue that remains is to determine how the compute commands that the controller relies on will operate (eg. compute complexity ...). As we note above, the controller will now maintain the revision throughout its processing and thus needs to pass a target with .../specs/{spec}@{revision} to the executed command. When we add this behavior, I assume the commands should also allow the "latest" Spec level - assuming the latest revision when a revision is not specified, correct?

shrutiparabgoogle · 2022-10-31T23:15:39Z

As per the discussion with Tim and Scott, we all concluded that we need to support optional spec revisions in the registry tool commands.

The current behavior of the API has an optional use of revisionID, example:

projects/demo/locations/global/apis/petstore/versions/v1/specs/openapi.yaml/artifacts/conformance-report -> refers to the artifact on the latest revision.
projects/demo/locations/global/apis/petstore/versions/v1/specs/openapi.yaml@abcdef/artifacts/conformance-report -> refers to the artifact on a specific revision.
We should support similar behavior in our registry tool.

We have a couple of commands in the registry tool which take as input a spec and generates an artifact. The current and expected behavior of such commands should be as follows:
Take for example the following command: registry compute conformance <input>

1. Spec Collection Input: projects/demo/locations/global/apis/-/versions/-/specs/-
Current behavior:
Compute conformance for all the specs (latest revision) in the registry and attach the result as an artifact to the spec.
Expected behavior:
Compute conformance for all the specs (latest revision) in the registry and attach the result as an artifact to the latest specRevision.

2. Spec Input: projects/demo/locations/global/apis/petstore/versions/v1/specs/openapi.yaml
Current behavior:
Compute conformance for the specified spec's latest revision and attach the result as an artifact to the spec.
Expected behavior:
Compute conformance for the specified spec's latest revision and attach the result as an artifact to the latest specRevision.

3. SpecRevision Collection Input: projects/demo/locations/global/apis/-/versions/-/specs/-@-
Current behavior:
Unsupported
Expected behavior:
Compute conformance for all the specs (all revisions) in the registry and attach the result as an artifact to the the corresponding specRevisions.

4. SpecRevision Input: projects/demo/locations/global/apis/petstore/versions/v1/specs/openapi.yaml@abcdef
Current behavior:
Unsupported
Expected behavior:
Compute conformance for the specified revision and attach the result as an artifact to the specRevision.

The list of commands which will need the update mentioned above:

registry compute
registry delete
registry get
registry list
registry label
registry upload
Some of these commands might already support the artifact revision pattern, verify if they behave as expected.

shrutiparabgoogle assigned theganyo Oct 6, 2022

timburks changed the title ~~Registry: Supporting artifacts on resource revisions~~ Registry server: Supporting artifacts on resource revisions Oct 6, 2022

timburks changed the title ~~Registry server: Supporting artifacts on resource revisions~~ Registry API: Supporting artifacts on resource revisions Oct 6, 2022

timburks added the enhancement New feature or request label Oct 20, 2022

theganyo mentioned this issue Oct 20, 2022

parent spec artifacts on spec revisions #794

Merged

theganyo mentioned this issue Nov 7, 2022

artifacts on deployment revisions #818

Merged

theganyo closed this as completed in #818 Nov 7, 2022

theganyo mentioned this issue Jan 23, 2023

ListRevisions is not working as expected. #960

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Registry API: Supporting artifacts on resource revisions #774

Registry API: Supporting artifacts on resource revisions #774

shrutiparabgoogle commented Oct 6, 2022 •

edited

theganyo commented Oct 10, 2022

shrutiparabgoogle commented Oct 10, 2022

timburks commented Oct 10, 2022

theganyo commented Oct 12, 2022

theganyo commented Oct 13, 2022

theganyo commented Oct 19, 2022 •

edited

theganyo commented Oct 20, 2022 •

edited

theganyo commented Oct 20, 2022

shrutiparabgoogle commented Oct 20, 2022

shrutiparabgoogle commented Oct 26, 2022

theganyo commented Oct 26, 2022

theganyo commented Oct 27, 2022

shrutiparabgoogle commented Oct 31, 2022

Registry API: Supporting artifacts on resource revisions #774

Registry API: Supporting artifacts on resource revisions #774

Comments

shrutiparabgoogle commented Oct 6, 2022 • edited

theganyo commented Oct 10, 2022

shrutiparabgoogle commented Oct 10, 2022

timburks commented Oct 10, 2022

theganyo commented Oct 12, 2022

theganyo commented Oct 13, 2022

theganyo commented Oct 19, 2022 • edited

theganyo commented Oct 20, 2022 • edited

theganyo commented Oct 20, 2022

shrutiparabgoogle commented Oct 20, 2022

shrutiparabgoogle commented Oct 26, 2022

The target resource needs an update

The target resource doesn't exist yet, needs creation

theganyo commented Oct 26, 2022

theganyo commented Oct 27, 2022

shrutiparabgoogle commented Oct 31, 2022

shrutiparabgoogle commented Oct 6, 2022 •

edited

theganyo commented Oct 19, 2022 •

edited

theganyo commented Oct 20, 2022 •

edited