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

Emiel/add type property to metrics #24

Merged
merged 4 commits into from
Dec 11, 2024

Conversation

emielver
Copy link
Collaborator

@emielver emielver commented Dec 11, 2024

A customer has noticed that adding the type property to a .yaml file causes the metric sync to error out with the following message:

Schema violation in **.yaml: 
Additional properties are not allowed ('type' was unexpected)

Meanwhile, the yml they were trying to sync was gathered using the COPY YAML button on Eppo's UI, and the API is able to handle a type field. So here we introduce a type field to the schema, and make it an ENUM accepting only the values allowed currently when creating a new metric in the Eppo UI.

@emielver emielver requested a review from chasdevs December 11, 2024 11:44
@MaloChevillotte
Copy link
Contributor

Looks like your IDE reformatted eppo_metrics_sync/schema/eppo_metric_schema.json.
It is difficult to see the changes actually made to this file and hence to get all the changes from the PR.
Can you reformat this file so that changes are easy to read?

@MaloChevillotte MaloChevillotte self-requested a review December 11, 2024 14:53
@emielver
Copy link
Collaborator Author

@MaloChevillotte
The actual changes in the eppo_metrics_sync/schema/eppo_metric_schema.json are shown here in the light green (lines 127-130 and L153). I think removing the formatting will be a bit tricky, will this suffice for now?
image

@MaloChevillotte
Copy link
Contributor

@emielver I understand that it is annoying but if your IDE this problem will pop up every time you push a commit with a json file in it. The sooner we fix this, the better.
Can you try reverting commit 7676b9d with one commit and re-commiting afterwards with proper formatting?

@emielver emielver force-pushed the emiel/add-type-property-to-metrics branch from a22c6a4 to 81a455a Compare December 11, 2024 16:04
@emielver
Copy link
Collaborator Author

@MaloChevillotte I did some rebasing magic and now it should be working with the appropriate commits

@emielver emielver merged commit befe6d3 into main Dec 11, 2024
1 check passed
@emielver emielver deleted the emiel/add-type-property-to-metrics branch January 14, 2025 02:48
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