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

Golang library for validation: foundation #12

Merged
merged 13 commits into from
Aug 10, 2020

Conversation

ycombinator
Copy link
Contributor

This PR creates the foundations for a Golang library for package validation. Concretely, it creates the following:

  • Makefile(s) for building the library, which includes building the code but also bundling the package specifications into the library, and

  • a go module that provides an API for validating a package against the appropriate specification version. This API can be used as follows:

    import "github.com/elastic/package-spec/code/go/pkg/validator"
    
    validationErrs := validator.ValidateFromPath("/path/to/package/root/folder")

    where validationErrs will implement the error interface type.

Note that this PR does not implement the actual validation logic yet. That is, calling the API will not return any validation errors at the the moment. The actual validation logic will be implemented in follow up PRs.

Related: #4

package validator

import (
"github.com/elastic/package-spec/code/go/internal/validator"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite convinced about the import path as it contains many package levels. Ideally it would be github.com/elastic/package-spec/validator, but I assume that code/ will contain implementation also for JS.

Fine for me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++ I'd love for it to be simpler too but I don't know how to make that happen. Indeed the reason was to allow for other language libraries to be added in the future (I copied the structure from ECS: https://github.com/elastic/ecs/tree/master/code).

Copy link
Contributor

Choose a reason for hiding this comment

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

Just thinking loud: you can leave specification files in package-spec and depend on them in implementations: go-package-validator, js-package-validator. But I believe it might be an overkill here as it's relatively small.

As I said, I'm happy to leave it as is or add a single facade file on repo root (so all imports would be: github.com/elastic/package-spec).

Copy link
Contributor Author

@ycombinator ycombinator Aug 10, 2020

Choose a reason for hiding this comment

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

Just thinking loud: you can leave specification files in package-spec and depend on them in implementations: go-package-validator, js-package-validator. But I believe it might be an overkill here as it's relatively small.

You mean pull out the libraries into their own repos? Or something else?

As I said, I'm happy to leave it as is or add a single facade file on repo root (so all imports would be: github.com/elastic/package-spec).

This could work but I'm not sure what this means for other languages. I think maybe this is something we can revisit once we add JS validation too? I know it would mean updating existing clients at that point but hopefully that's okay since adding a facade would be a backwards-compatible change (at least for some period of time).


// ValidateFromPath validates a package located at the given path against the
// appropriate specification and returns any errors.
func ValidateFromPath(packageRootPath string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you run this as static method. I admit I didn't go deeper yet and might be wrong, but do you cache loaded specs in some global validator? just not to load schemas every single time, a user validates a package.

Copy link
Contributor Author

@ycombinator ycombinator Aug 10, 2020

Choose a reason for hiding this comment

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

Yes, I'm using https://github.com/rakyll/statik which results in the code/go/internal/spec/statik.go being generated from the "raw" YAML spec files when make update is called. Then that file gets compiled in with the rest of the library code. So by the time the validation library is called, the specs are in memory already.

Copy link
Contributor

Choose a reason for hiding this comment

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

With statik you can keep raw files in memory, but you need to initialize structures with loaded data. I was thinking about following this model:

  1. Load all specs from in-memory filesystem.
  2. Build validators once.
  3. Use validators against packages.

I understand it might be a pre-optimization, but hopefully may save memory by not initializing validators on every call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this optimization is definitely beyond the scope of this PR. This PR is mainly about exposing the right API and making sure spec files are bundled in with the library (using statik).

So I would like to consider this optimization in future PRs, e.g. #14. Even then I would consider this a premature optimization and try to defer it to a time when we actually need it, e.g. start to see a perceptible slowness or larger-than-expected memory utilization when running elastic-package lint. Until then I would prefer to prioritize getting the API right and a functioning implementation — we can always improve the implementation later, as needed.

"strings"
)

// ValidationErrors is an Error that contains a iterable collection of validation error messages.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: an error
nit: an iterable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c0fcc8d.

code/go/internal/spec_test.go Show resolved Hide resolved
@ycombinator ycombinator force-pushed the lib-golang-foundation branch from c0fcc8d to 1e85eb7 Compare August 10, 2020 17:01
@elasticmachine
Copy link

elasticmachine commented Aug 10, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #12 updated]

  • Start Time: 2020-08-10T17:13:17.917+0000

  • Duration: 2 min 14 sec

@ycombinator ycombinator merged commit 6ef6846 into elastic:master Aug 10, 2020
@ycombinator ycombinator deleted the lib-golang-foundation branch August 10, 2020 17:15
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.

3 participants