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

add schemaFile to ValidationFile #2206

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Conversation

kartikaysaxena
Copy link
Contributor

Related authzed/zed#453

Added schemaFile to ValidationFile which could be used to specify path of the schema.

@tstirrat15
Copy link
Contributor

@kartikaysaxena is this all it took for SpiceDB changes? I was worried that a missing schema: key would cause the validation file parsing for that key to fail if it wasn't present.

@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Jan 14, 2025
@tstirrat15
Copy link
Contributor

The author has validated that this change works as desired over in authzed/zed#455

@kartikaysaxena
Copy link
Contributor Author

@kartikaysaxena is this all it took for SpiceDB changes? I was worried that a missing schema: key would cause the validation file parsing for that key to fail if it wasn't present.

Added a check if schema and schemaFile aren't specified, also, afaik it panics when schema is not present and we try to compile it in zed, updated the checks there too.

@tstirrat15
Copy link
Contributor

It looks like there are some lint and validation failures - can you have a look at those?

@tstirrat15
Copy link
Contributor

There's still a test failure. I think it's related to the new parsing you implemented - I was thinking of putting that validation up at the level of zed rather than down here in the parser. I think what you had before with just adding the SchemaFile key to the struct was probably fine.

@kartikaysaxena
Copy link
Contributor Author

There's still a test failure. I think it's related to the new parsing you implemented - I was thinking of putting that validation up at the level of zed rather than down here in the parser. I think what you had before with just adding the SchemaFile key to the struct was probably fine.

Yes now the parser enforces that there must be either schema or schemafile in the file, and so the test expects the same, sure would remove that.

@tstirrat15
Copy link
Contributor

Yeah, I'm okay with that. We can either drop that validation for now or do it in a different way.

@tstirrat15 tstirrat15 enabled auto-merge January 14, 2025 22:05
tstirrat15
tstirrat15 previously approved these changes Jan 14, 2025
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

LGTM

@tstirrat15 tstirrat15 disabled auto-merge January 14, 2025 22:06
@tstirrat15
Copy link
Contributor

sorry, one more thing - can you squash the commits? I usually do it with git rebase -i main and then reword the first commit and fixup the others. I'll merge it after that.

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

LGTM

@tstirrat15 tstirrat15 enabled auto-merge January 14, 2025 22:45
@tstirrat15 tstirrat15 added this pull request to the merge queue Jan 14, 2025
Merged via the queue into authzed:main with commit a80f596 Jan 14, 2025
38 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants