-
Notifications
You must be signed in to change notification settings - Fork 12
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
Update test case for empty dir hash in gen dir #1024
Conversation
@@ -37,7 +37,7 @@ type Plugin struct { | |||
} | |||
|
|||
func (p *Plugin) String() string { | |||
return fmt.Sprintf("%s:%s", p.Identity.IdentityString(), p.Version) | |||
return fmt.Sprintf("%s:%s", p.Identity.IdentityString(), p.PluginVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version here is v1
of the config, it should be the plugin version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh - good catch!
@@ -37,7 +37,7 @@ type Plugin struct { | |||
} | |||
|
|||
func (p *Plugin) String() string { | |||
return fmt.Sprintf("%s:%s", p.Identity.IdentityString(), p.Version) | |||
return fmt.Sprintf("%s:%s", p.Identity.IdentityString(), p.PluginVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh - good catch!
// plugins. | ||
allowedEmptyPluginSums = map[string]bool{ | ||
"buf.build/bufbuild/validate-java": true, | ||
"buf.build/grpc-ecosystem/gateway": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add an exception for thebuf.build/community/mercari-grpc-federation
plugin for now until a fix is made available (at least for the current version)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to undo and remove that plugin entirely, and re-open the issue advocating for an upstream fix. Any concerns with that?
It'll eliminate a broken plugin, and we also don't know if they'll fix it upstream. So best to remove and re-open issue imo.
This PR adds a defensive check in our plugin tests to avoid passing plugins that generate no code and produce an "empty dir" hash.
There is a historical allowlist of plugins we'll allow, but moving forward we should craft protos to exercise valid code generation, in a similar way as:
https://github.com/bufbuild/plugins/blob/d524f5ffe0e922ac10752db571b60a320f7fc7f4/tests/plugins_test.go#L133-L13
For context, this would help catch #1017 against the typical images we pass in testing.