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

Fix proposal for #319 #327

Merged
merged 17 commits into from
Aug 21, 2023
Merged

Conversation

gfenoy
Copy link
Contributor

@gfenoy gfenoy commented May 30, 2023

This changes should solve the issue #319 related to moving the schemas location from the extension specific openapi subdirectory to root.

@jerstlouis
Copy link
Member

jerstlouis commented May 30, 2023

Thanks @gfenoy !

We should also add the (POST) paths for processes-dru to openapi/paths and then include them in ogcapi-processes.yaml. Then if it works with your demo API, we could add your server as an example supporting DRU to the list of example servers.

You could also potentially use the example OpenAPI definition to assemble your API definition for your implementation, with swagger-cli bundle ogcapi-processes.yaml. Simply comment out any paths that your API doesn't implement, tweak the process-list.json to the processes your API supports (or return it dynamically from your server at/api/process-list), and add any extra things that you support as custom extensions or from other OGC APIs (several of which now following this structure).

@gfenoy
Copy link
Contributor Author

gfenoy commented May 30, 2023

I have made the renaming using the r prefix for responses

@gfenoy
Copy link
Contributor Author

gfenoy commented May 30, 2023

Thanks @jerstlouis, it is a very nice way to validate our changes to use the swagger-cli bundle ogcapi-processes.yaml command.

@jerstlouis
Copy link
Member

jerstlouis commented May 30, 2023

@gfenoy Now you can add your server to the list of examples and say it supports DRU :) Then we can test it all directly from SwaggerUI. You can also do swagger-cli validate ogcapi-processes.yaml. And sometimes the SwaggerUI will find more problems (bottom icon will say invalid). You can just point SwaggerUI (https://petstore.swagger.io/?url=) to the bundled JSON file.

@gfenoy
Copy link
Contributor Author

gfenoy commented May 30, 2023

@jerstlouis should I also push the bundle to replace the current one?

I ask this question because I noticed that there is no mention of Part 3 in the ogcapi-processes.yaml.

Also, we may think of having the following files:

  • openapi/ogcapi-processes-1.yaml: include the current ogcapi-processes.yaml file (before the modifications I made)
  • openapi/ogcapi-processes-2.yaml: include the ogcapi-processes.yaml offering both part 1 and part 2 definitions
  • openapi/ogcapi-processes-3.yaml: include the ogcapi-processes.yaml offering both part 1, part 2 and part 3 definitions

What do you think?

@jerstlouis
Copy link
Member

jerstlouis commented May 30, 2023

@gfenoy Yes you can update the updated bundle.

t there is no mention of Part 3 in the ogcapi-processes.yaml.

That's because Part 3 does not really define any new paths, it only extends the execution request and defines the response=collection/landingPage on the POST to /execution.

Also, we may think of having the following files:

I think it's simpler to just include everything in a single ogcapi-processes.yaml, and developers can comment out the paths they do not implement when they bundle their own OGC API.
This is why I suggest it would be great if you can add your server which implements Part 2 as an additional API deployment example, since our server that is currently the only one there in the list only supports Part 1 and 3.

@gfenoy
Copy link
Contributor Author

gfenoy commented May 30, 2023

This is why I suggest it would be great if you can add your server which implements Part 2 as an additional API deployment example

I will proceed after we get confirmation that once deployed a Server Instance relying on the proposed new schemas is properly working.

Add short documentation on how to generate the desired files.

Small fix to make operationId unique
Add instructions to generated the files in the README

Thanks @jerstlouis for the comment about this
@@ -8,7 +8,7 @@ The list of supported paths should be ajusted in `ogcapi-processes-3.yaml`.

The `ogcapi-processes-3.bundled.json` was generated with `swagger-cli bundle` from `ogcapi-processes-3.yaml` and its dependencies included from the components sub-directories.

The `paths/processes-dru/pProcessListDeploy.yaml` and `paths/processes-dru/pProcessDescriptionReplaceUndeploy.yaml` files are generated by concatening multiple files in a single one. you can sue the command bellow to update their value with the current schema:
The `paths/processes-dru/pProcessListDeploy.yaml` and `paths/processes-dru/pProcessDescriptionReplaceUndeploy.yaml` files are generated by concatening multiple files in a single one. you can use the command bellow to update their value with the current schema:
Copy link
Member

@jerstlouis jerstlouis May 31, 2023

Choose a reason for hiding this comment

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

Thanks @gfenoy . bellow is still wrong though :) (below)

Copy link
Contributor Author

@gfenoy gfenoy May 31, 2023

Choose a reason for hiding this comment

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

Thanks for your patience @jerstlouis

Copy link
Member

Choose a reason for hiding this comment

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

@gfenoy No thank you for yours ;) Sorry for being so annoying finding things to fix :P

@@ -0,0 +1,5 @@
name: w
Copy link
Member

Choose a reason for hiding this comment

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

@gfenoy What's w for? Can we add a description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jerstlouis it is used to point to the workflow id to be deployed from a CWL containing multiple workflow definitions.

I have added the description, I hope it is clear.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

@fmigneault
Copy link
Contributor

@gfenoy
See #329 for the CWL schema discussed in #319.

@gfenoy
Copy link
Contributor Author

gfenoy commented Jun 7, 2023

@fmigneault following the discussion there.

My understanding is that the schema will be made available from there https://github.com/common-workflow-language/cwl-v1.2/.

I don't know yet if we we plan to keep a copy of the schema also in this repository. I would like to have a confirmation about this.

If not, then I will wait for the schema to be made available there to update the schema to reference this schema.

@fmigneault
Copy link
Contributor

@gfenoy
I think it would be better to use the published external reference to make sure it stays up-to-date with any fixes applied on the CWL side.

@fmigneault
Copy link
Contributor

@fmigneault
Copy link
Contributor

fmigneault commented Jul 19, 2023

@gfenoy

In these two definitions, I recall having a nested unit or href mapping that allowed respectively to either pass the definitions direct;y as JSON, or provide an external reference to a document containing the execution unit contents.

https://github.com/GeoLabs/ogcapi-processes/blob/a8347b5f2c9e37e4581b68739120e62c4ea1317a/openapi/schemas/processes-dru/ogcapppkg-array.yaml#L3-L6

  oneOf:
    - $ref: "executionUnit.yaml"
    - $ref: "../common-core/link.yaml"
    - $ref: "../processes-core/qualifiedInputValue.yaml"

https://github.com/GeoLabs/ogcapi-processes/blob/a8347b5f2c9e37e4581b68739120e62c4ea1317a/openapi/schemas/processes-dru/ogcapppkg.yaml#L7-L12

  executionUnit:
    oneOf:
      - $ref: "executionUnit.yaml"
      - $ref: "../common-core/link.yaml"
      - $ref: "../processes-core/qualifiedInputValue.yaml"
      - $ref: "ogcapppkg-array.yaml"

The href variant would be handled by "../common-core/link.yaml", but the unit one is missing.

Also, https://raw.githubusercontent.com/common-workflow-language/cwl-v1.2/1.2.1_proposed/json-schema/cwl.yaml can now be added to those lists as well.

Thanks @fmigneault for this great work.

Co-authored-by: Francis Charette-Migneault <[email protected]>
@fmigneault
Copy link
Contributor

@gfenoy Is there something still blocking this PR?

@gfenoy
Copy link
Contributor Author

gfenoy commented Aug 21, 2023

@fmigneault we were discussing about merging this PR during the last SWG meeting and no objection was raised.

I plan to attend the today meeting and hope we can get this PR merged during the SWG meeting except if an objection is exposed.

In case you have time, it would be great to have you attending the meeting to support this PR.

@pvretano
Copy link
Contributor

21-AUG-2023: SWG reviewed the PR and agreed to merge. If any issues arrive after the fact, new issues can be created to address those.

@pvretano pvretano merged commit 1297e3b into opengeospatial:master Aug 21, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants