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 github workflow to validate samples #104

Merged
merged 14 commits into from
Jul 13, 2023

Conversation

e-lo
Copy link
Contributor

@e-lo e-lo commented Feb 1, 2023

This PR adds a GitHub workflow for sample validation and adds a script to locally validate schemas and samples as well as lint.

Also:

  • linting
  • added requirements for jsonschema
  • updated documentation

Uses frictionlessdata/repository - which doesn't allow flag `--schema-sync` so will likely need to modify
@e-lo e-lo added 🧹 chore 💻 code Pertains to the infrastructure code labels Feb 1, 2023
@e-lo e-lo requested a review from botanize February 1, 2023 17:40
@e-lo e-lo self-assigned this Feb 1, 2023
@e-lo e-lo mentioned this pull request Feb 1, 2023
@e-lo
Copy link
Contributor Author

e-lo commented Feb 1, 2023

While it doesn't seem like there is the ability to add --schema-sync flag to the frictionless repository action at this time, they have created an issue for it.

You can at this time add an inquiry to the repository action, which in turn can skip various errors such as extra-label. That said, I don't see a way to make sure that optional fields in the schema aren't flagged...

Copy link
Contributor

@botanize botanize left a comment

Choose a reason for hiding this comment

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

Ok, glad they're looking into it. I don't think we should merge this workflow until there's a mechanism to specify --schema-sync, otherwise it will just produce huge reports (1 line per row of the csv!) full of spurious errors.

The alternative is to run frictionless validate --schema-sync directly in the workflow instead of using the frictionless repository workflow.

Copy link
Contributor

@botanize botanize left a comment

Choose a reason for hiding this comment

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

Hi! Overall this is good, but a few specific issues that need to be dealt with.

  • move away from deprecated set-output command
  • deal with dependencies for test script
  • fix jsonschema-cli validation step

See other comments for suggestions.

Joey

.github/workflows/validate_samples.yml Outdated Show resolved Hide resolved
.github/workflows/validate_samples.yml Outdated Show resolved Hide resolved
.github/workflows/validate_samples.yml Outdated Show resolved Hide resolved
.github/workflows/validate_samples.yml Outdated Show resolved Hide resolved
.github/workflows/validate_samples.yml Show resolved Hide resolved
test Outdated Show resolved Hide resolved
test Outdated Show resolved Hide resolved
e-lo added 3 commits July 12, 2023 10:51
Script will find all executables in /tests and run them.

validate_profile script now downloads the json-schema for profiles from URL

Also:
- update docs
- fix script
Copy link
Contributor

@botanize botanize left a comment

Choose a reason for hiding this comment

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

Some lingering errors in tests

tests/validate_samples Outdated Show resolved Hide resolved
tests/test_all Outdated Show resolved Hide resolved
tests/test_all Outdated Show resolved Hide resolved
e-lo added 3 commits July 12, 2023 11:54
set-output --> $GITHUB_ENV
external placeholder.com --> octocons
+ schema-sync
@e-lo
Copy link
Contributor Author

e-lo commented Jul 12, 2023

@botanize I think i got all your comments addressed in subsequent commits minus the one suggesting that only do validation on the files that actually changed (we can address that when we have files to do).

@e-lo e-lo requested a review from botanize July 12, 2023 19:17
Copy link
Contributor

@botanize botanize left a comment

Choose a reason for hiding this comment

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

I'd still like to test this on actual sample data, but everything else seems to work, so let's merge.

@e-lo e-lo merged commit 9624947 into TIDES-transit:main Jul 13, 2023
0 of 2 checks passed
@e-lo e-lo deleted the add-validation-workflow branch July 13, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 chore 💻 code Pertains to the infrastructure code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants