Safely load metadata files. #19
Conversation
| try: | ||
| with open(config / "sdks.yaml", encoding="utf-8") as file: | ||
| meta = yaml.safe_load(file) | ||
| sdks, errs = parse_sdks("sdks.yaml", meta) | ||
| self.sdks = sdks | ||
| self.errors.extend(errs) | ||
| except Exception: | ||
| pass | ||
|
|
||
| try: | ||
| with open(config / "services.yaml", encoding="utf-8") as file: | ||
| meta = yaml.safe_load(file) | ||
| services, service_errors = parse_services("services.yaml", meta) | ||
| self.services = services | ||
| self.errors.extend(service_errors) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
If these silently fail then you'll have empty sdks and services, which will blow up later. Better to error out here.
There was a problem hiding this comment.
That's the default state, and there are clear error messages then. In fact, most tributaries are not expected to have sdks or services, so they need to be empty and need to inherit from the parent doc_gen.
There was a problem hiding this comment.
If these files don't exist at all, even in the root repo, the first place I see an error happening is in metadata.parse_services, where it will raise an UnknownService exception. This seems a lot harder to track down than "no services defined" or something in this doc_gen function. I see that you don't want to error out on a missing file, but I think you'll want to check that at least some sdks and services exist before you start the parse_examples loop.
There was a problem hiding this comment.
It's not an error here until the consumer of for_root says it is. doc_gen.py doesn't care if there are services or sdks or validation. Only consumers do.
There was a problem hiding this comment.
But it is an error here if there are no services loaded at all. In this function it eventually calls parse_examples, which ultimately calls metadata.parse_services, which will fail with UnknownService. I think this is a bad experience and easily overcome by checking that sdks and services are not None or empty. I see that this is unlikely in that the main root would have to omit one of these files, so arguably "it will never happen", although saying that means it is now guaranteed to happen. If we just need to get this in today that's fine, I don't want to block on this.
DavidSouther
force-pushed
the
bug/safe_load_metadata
branch
from
804e1db
to
243dfba
Compare
DavidSouther
force-pushed
the
bug/safe_load_metadata
branch
from
243dfba
to
b321240
Compare
DavidSouther
force-pushed
the
bug/safe_load_metadata
branch
from
f033cfa
to
e01c0ad
Compare
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.