schema: add checkPlatform

[zhouhao3]

Fixes opencontainers/image-tools#39
reopen #469
Signed-off-by: zhouhao [email protected]

[zhouhao3]

I put the wrong message into a warning, it will not be terminated into operation.

[vbatts]

@q384566678 i don't parse your last comment, is the "wrong message" something we need to wait for you to get correct?

[zhouhao3]

@vbatts My last comment is on the update after the end of the explanation,In the #469 ：return fmt.Errorf("Combination of %q and %q is invalid.", OS, Architecture). In thsi PR, I change it to :fmt.Printf("waring: combination of %q and %q is invalid.", OS, Architecture).

[wking]

“waring” → “warning”

[wking]

“waring” → “warning” and “operation system” → “operating system”.

[wking]

The platform check you're adding here has nothing to do with “descendants”. Can we get a more generic name for this map? I'm still partial to calling the map of validators Validators ;).

[zhouhao3]

@stevvooe @vbatts PTAL

[zhouhao3]

@stevvooe @vbatts @jbouzane @jonboulle PTAL

[philips]

@q384566678 I am confused by the "Descendants" naming, can you explain?

[zhouhao3]

@philips The "Descendants" naming is wrong ,My idea is to validate their respective JSON files.
updated.

[philips]

I feel like these should all be errors not printf's.

[wking]

I feel like these should all be errors not printf's.

They aren't errors though, since the spec only SHOULDs a platform/OS supported by runtime-spec's platform.*.

It would be nice to have a more programmatic API for logging warnings, even if it was only “use this Write-supporting object to log errors” instead of hardcoding stdout, but I think that can probably happen in a follow-up commit.

[zhouhao3]

According to the specification, here should be a warning, not an error. (Although I think it should be output errors)

[stevvooe]

Why is a library function writing to stdout?

I am almost ready to propose a moratorium on validation until someone starts doing some engineering work. This is a huge waste of time for the maintainers and this stuff isn't passing muster.

[wking]

I am almost ready to propose a moratorium on validation until someone starts doing some engineering work.

We just need a way to handle warnings for SHOULD violations (they aren't errors, but the caller may want to know about them). I've tried floating strict (#403, opencontainers/image-tools#66) and TAP comments (opencontainers/image-tools#60). And @xiekeyang has been trying to follow your guidance on this in #452. So far nobody has managed to land anything more polished than “print warnings to stdout” (and we have precedence for that approach in master). So until the image-spec maintainers decide how they want SHOULD violations handled, I think we should just roll forward with fmt.Printf("warning:…. It will be easy enough to grep and replace once we have a better approach figured out.

[stevvooe]

@wking Printing to stderr/stdout makes this package completely useless for any case where I care about program output.

So until the image-spec maintainers decide how they want SHOULD violations handled

Propose something that isn't broken.

[wking]

Propose something that isn't broken.

It's not my project ;). I'll let you all sort this out, but it seems unfortunate to block otherwise fine PRs because they follow an established-in-master pattern.

[stevvooe]

@wking These packages are unusable in other commands that control stderr/stdout. I am not sure how they slipped into master, but this is pretty bad.

I am going to take a deeper look into this next week, so I'll be able to provide some better direction.

[vbatts]

@stevvooe can you make a decision here so we can close this out?

[stevvooe]

@vbatts The problem here is that these functions are sending content over stdout. I understand that this PR is addressing another issue (platform validation), but this situation is less than ideal.

I'd like to see an API like this:

if err := validate(content); err != nil {
  for _, warning := range Warnings(err) {
    // do something with warnings
  }
}

We could define interfaces like this:

type Warnings {
  Warnings() []error
}

type Errors {
  Errors() []error
}

Either of these could be asserted on the error returned from the validate function. We can define helpers for common cases. For example, the below would not take the error path when there are just warnings:

if err := validate(content); err != nil && IgnoreWarnings(err) {
   // handle hard errors
}

I suppose that this could be rebased and merged then followed up with a PR that places in a proper validation system.

[vbatts]

so, even though the validation already had printf's for output warning, and this PR just continues that usage, the wish is to not continue using printf's but instead switch to returning a custom error type that satisfies interfaces so that the warnings and errors can be customized for formatted output, counting, etc.

I'm good with that.

@q384566678 do you want to rebase this or close it out and allow for a fresh approach?

[wking]

…a custom error type that satisfies interfaces so that the warnings and errors can be customized…

There's something like that in flight with opencontainers/runtime-tools#354 if folks are interested in sharing the same implementation across OCI projects. You'd need to add a new level for “not currently supported by the test suite”, but that seems fairly straightforward.

[stevvooe]

@vbatts I hope that is a reasonable position. The current validation code cannot be consumed by other projects. Adding more validation code that takes this approach seems odd when we can fix it.

[zhouhao3]

@vbatts I'm going to do the rebase first, and then I'm going to refine warings.

[zhouhao3]

After errors and warnings return, errors is output through t.Errorf, the warning output I still do not know, or through fmt.printf output?

[stevvooe]

@q384566678 It needs to be like I described in #491 (comment). These should not have anything to do with output. The output should be controlled by the caller.

[stevvooe]

LGTM

We'll fix the problems with validation in the course of #686.

[vbatts]

LGTM