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

Run OPTIMADE validator for Netlify PR instance #57

Merged
merged 15 commits into from
Jan 18, 2021

Conversation

CasperWA
Copy link
Member

@CasperWA CasperWA commented Jan 6, 2021

Use the OPTIMADE validator GitHub Action on the Netlify instance created for a PR to validate the providers as an index meta-database.

To do:

  • Test also all locally hosted index meta-databases.
  • Possibly also test all externally hosted index meta-databases.

@CasperWA
Copy link
Member Author

CasperWA commented Jan 6, 2021

@ml-evs I think the error occurring here is not the fault of the index meta-database, but rather our validator (see here).

Basically it shouldn't rely on the unversioned base URL endpoints, but rather the mandatory vMAJOR-versioned base URL endpoints.

@CasperWA
Copy link
Member Author

CasperWA commented Jan 6, 2021

Now it seems this PR is blocked by Materials-Consortia/optimade-python-tools#659.

@ml-evs
Copy link
Member

ml-evs commented Jan 6, 2021

The new error about missing provider prefixes should strongly encourage you to review and merge Materials-Consortia/optimade-python-tools#659 😁

@ml-evs
Copy link
Member

ml-evs commented Jan 6, 2021

Right, so the new error is because netlify has a rich 404 page that doesn't have any JSON...

@ml-evs
Copy link
Member

ml-evs commented Jan 6, 2021

@ml-evs
Copy link
Member

ml-evs commented Jan 7, 2021

Looking again at this, the validator itself runs correctly for the versioned URL without any changes; since the HTML 404 page still returns the expected 404 status code (duh), no attempt is made to deserialize to JSON in the validator, so the PR 665 linked above isn't super necessary (though it might be nice to have to give a more instructive error message when the 404 isn't expected).

The problem here is that the validator action is taking the versioned URL as the base URL and is appending another version to it. You can see in the logs that the first validation goes fine, but the second fails. I think this should be clarified in the validator action and will raise an issue there. For the meantime here, we can try the setting all_versioned_paths: false. This isn't suitable, instead we need something like a "dont run unversioned path" or version negotiation in the validator directly.

@CasperWA
Copy link
Member Author

CasperWA commented Jan 7, 2021

all versioned paths is by default set to False.
I don't think I'm completely following the issue, but if it's a problem of the validator action, please create an issue there. Never mind, just saw you created Materials-Consortia/optimade-validator-action#52 👍 Cheers.

@CasperWA
Copy link
Member Author

The current reason for the validator failing is a real reason. Issue #58 has been opened to track this.

@CasperWA
Copy link
Member Author

CasperWA commented Jan 15, 2021

I have now done stuff.
This now seems like a bit of being in a place between two worlds.
So what was called "Scheduled validator" wasn't set up to run on a schedule. I suspect this is because it's a copy-paste job from providers-dashboard.

Instead I have now separated the actual OPTIMADE validator tests into a separate workflow, which tests the Netlify deployed preview, both the list of providers index meta-db, as well as the locally hosted index meta-dbs.
This is done for all PRs.

Then the existing validation (pytest run) still runs for all PRs and all pushes. But I'm pretty sure the pytests are running similar tests as the OPTIMADE validator is running. So I'm suspecting we should simply remove it. But as I'm not 100 % sure, I'd rather do an all-in at this point.

@CasperWA CasperWA requested a review from ml-evs January 15, 2021 21:14
@CasperWA CasperWA marked this pull request as ready for review January 15, 2021 21:15
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

LGTM, nice one @CasperWA.

I think its fine to keep the sanity checks in for now (mostly as a sanity check for the validator itself). It's a shame the wait-for-it hasn't been turned into an action yet!

@CasperWA
Copy link
Member Author

(...) It's a shame the wait-for-it hasn't been turned into an action yet!

Actually, I haven't even checked. There might be something. But yeah, otherwise it could be a procrastination project 😅

@CasperWA CasperWA merged commit da74513 into Materials-Consortia:master Jan 18, 2021
@CasperWA CasperWA deleted the use_optimade-validator branch January 18, 2021 10:32
@ml-evs
Copy link
Member

ml-evs commented Jan 18, 2021

(...) It's a shame the wait-for-it hasn't been turned into an action yet!

Actually, I haven't even checked. There might be something. But yeah, otherwise it could be a procrastination project sweat_smile

I had a quick look but couldn't see anything, surprising given how widely that script is used

@CasperWA
Copy link
Member Author

(...) It's a shame the wait-for-it hasn't been turned into an action yet!

Actually, I haven't even checked. There might be something. But yeah, otherwise it could be a procrastination project sweat_smile

I had a quick look but couldn't see anything, surprising given how widely that script is used

Yeah! Okay. Well, I might set up something then. After hours :)

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.

None yet

2 participants