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

move everything out of internal #31

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

bhelx
Copy link
Contributor

@bhelx bhelx commented Nov 26, 2024

Move plugin and schema.json out of internal repo

This moves the schema.json out into the light and puts it with the rest of the parsing and validating code where it belongs. Since they change together and we should publish them together.

I removed checkfile related things from the plugin, we can decide whatto do about that. I also moved the JSON Schema checks to inside this library instead of being in the plugin. We should add more unit testing on this library to exercise and improve the JSON schema validation.

We can add some light XTP plugin tests to make sure that the plugin is still working as behaving, but not going so far as to create a lot of individual test cases that couple the wasm level tests to the implementation details of things like features and error messages.

This moves the schema.json out into the light and puts it with the rest
of the parsing and validating code where it belongs. Since they change
together and we should publish them together.

I removed checkfile related things from the plugin, we can decide what
to do about that. I also moved the JSON Schema checks to inside this
library instead of being in the plugin. We should add more unit testing
on this library to exercise and improve the JSON schema validation.

We can add some light XTP plugin tests to make sure that the plugin is
still working as behaving, but not going so far as to create a lot of
individual test cases that couple the wasm level tests to the
implementation details of things like features and error messages.
@bhelx bhelx marked this pull request as draft November 26, 2024 23:47
@bhelx
Copy link
Contributor Author

bhelx commented Nov 26, 2024

Note that the tests are broken because the error messages are different. but this reflects what the user will actually see.

}

/**
* Checks if the input schema has any imports
Copy link
Contributor Author

Choose a reason for hiding this comment

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

deciding if we need this or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhmd-azeez you'd be best to say. where is this used exactly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the only place we use this:

https://github.com/dylibso/xtp/blob/cf5cc59bc12750f9d6731b2f23276f044880d428/service/api/src/routes/api/v1/extension-points/index.ts#L109

We can probably replace it with a call to validate_schema

@@ -0,0 +1,6 @@
bin = "dist/plugin.wasm"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i simplified this doc

Comment on lines +401 to +402
const ajv = new Ajv();
const validate = ajv.compile(JSON_SCHEMA);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

doing the ajv validation inside the library now

required:
- exports
additionalProperties: false
$defs:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhmd-azeez I've made a lot of simplifications to the json schema. I suspect we can go further but not 100% sure yet. Curious if you could take a look.

@bhelx bhelx marked this pull request as ready for review December 3, 2024 16:52
@bhelx
Copy link
Contributor Author

bhelx commented Dec 3, 2024

Still some work to do but opening up for early review.

@@ -95,43 +101,6 @@ test('parse-v1-document', () => {
expect(exp.output?.contentType).toBe('application/json')
})

test('parse-v1-invalid-document', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As noted before, the errors are all different because of the way that json schema errors work. It's not the most pleasant experience, but this is the real one that users are having so at least we have a ground truth now. We'll need to get more granular about error testing here and we may need to rethink the strategy about the layered approach to error handling.

description: A set of all the enemies of pac-man
enum:
- blinky
- pinky
- inky
- clyde
ComplexObject:
type: object
Copy link
Contributor Author

Choose a reason for hiding this comment

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

you need to say the types now

versions:
v1:
status: Draft
changes:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: come back and add latest changes here

Copy link
Contributor

@mhmd-azeez mhmd-azeez left a comment

Choose a reason for hiding this comment

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

Nice! Moving schema validation to one place makes it way easier to maintain and update. Thank you very much!

AWS_SECRET_ACCESS_KEY: ${{ secrets.STATIC_S3_AWS_SECRET_ACCESS_KEY }}
AWS_DEFAULT_REGION: us-east-2
run: |
aws s3 cp ${{ github.workspace }}/schema.json s3://static.dylibso.com/schema.json --acl public-read
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning on keeping schema.yaml in the repo? if so, I don't see any script that generates schema.json from schema.yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

We also should publish schema-changelog.yml for future use

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.

2 participants