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

Remove validation warnings to stdout #1192

Merged

Conversation

sudo-bmitch
Copy link
Contributor

@sudo-bmitch sudo-bmitch commented Jun 19, 2024

Based on our discussion on the June 6th call, we decided to remove the warnings from the code and focus on only things that trigger an error. We had not been maintaining the existing warnings. Anything that only prints to stdout/stderr was deleted since it's unlikely anyone is using that. Private functions that only triggered a print to stdout were deleted.

If others want a stricter validation, those on the call felt it would be better for them to perform that themselves rather than adding new code for OCI to maintain.

Fixes #686.

Signed-off-by: Brandon Mitchell <git@bmitch.net>
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

Agreed, code that nobody is maintaining, ~nobody is using, and nobody (I'm aware of) is asking for is code that shouldn't exist, IMO!

@sajayantony sajayantony merged commit 39ab2d5 into opencontainers:main Jul 15, 2024
4 checks passed
@sudo-bmitch sudo-bmitch deleted the pr-validation-rm-warnings branch July 15, 2024 18:35
Sec32fun32

This comment was marked as spam.

@dontlaugh
Copy link

@sudo-bmitch any chance of getting tag for this soon? Even an rc.

Otherwise, if you're targeting a release next year, I can probably convince my team to pin to a commit. A tag would be nicer, though.

Thanks for fixing this so fast, btw.

@tianon
Copy link
Member

tianon commented Aug 26, 2024

Our tags are for versions/releases of the spec, not the Go code that comes along for the ride, so it's not likely we'd do a release just for this.

When we looked at this, we couldn't find many public consumers of this code; can you share what you're using it for and what value it's providing to you?

@dontlaugh
Copy link

dontlaugh commented Aug 26, 2024

so it's not likely we'd do a release just for this.

That's fine.

We use the function as one of our validation passes for image manifests, which is what we deal with most of the time in our implementation. Our API accepts a raw descriptor and this is a very early check to see if it's something we'll want to deal with.

	if err := schema.ValidatorMediaTypeManifest.Validate(reader); err != nil {
		return nil, err // bad request
	}

We do other validation (specific to our use case, like you all recommend) further down the call stack.

We aren't building an OCI-compliant API, but we do want to store compliant objects, so we're attracted to a method that checks json schema + some basic fields, and that's maintained upstream.

@sudo-bmitch
Copy link
Contributor Author

I like the idea of periodically cutting patch releases, it's good to exercise the process. But that process requires a 2/3 majority of the maintainers which is a relatively heavy lift. So my personal advice would be to pin to a commit if you need something soon.

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.

Fix the schema validation API
5 participants