-
Notifications
You must be signed in to change notification settings - Fork 300
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
Fix signature mismatches when steps have plugins #2319
Conversation
84f9f02
to
71229a9
Compare
fbc488e
to
c428fdd
Compare
c428fdd
to
ab13e10
Compare
78aaaa5
to
ed7d9ee
Compare
ed7d9ee
to
948bba6
Compare
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.
Nice, falling back to not altering the source is a good strategy.
|
||
u, err := url.Parse(p.Source) | ||
if err != nil { | ||
return p.Source |
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 wonder if there's a way that we can carry across that FullSource
had to fall back to being identical on a particular input. Then, when it comes time to sign the plugin, we can feed a warning to the job logs. It's perhaps not necessary right now, but I would want such a thing before we announce this is released.
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 think it's a bit tricky. Some inputs have to fall back because neither the pipeline-uploading agent nor the backend know what local or vendored plugins will be present. And some inputs must fall back because there needs to be a way to override the assumption that the plugin is in GitHub (and usually in buildkite-plugins). Failing to parse as a URL / URI could be a mistake, but I have the suspicion that some edge case will fall into it.
return p.Source | ||
} | ||
|
||
u, err := url.Parse(p.Source) |
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.
🤞 that there is no inconsistency with this and ruby's URI.parse
The first part of this PR enables
CommandStep
to be unmarshaled directly from JSON consistently. This is mainly needed becausePlugin
wasn't set up to be unmarshaled from JSON, but reusing the YAML -> ordered path keeps it consistent.The second part of this PR changes how
Plugin
is marshaled back to JSON.FullSource
applies similar normalisations that the backend should be applying to plugin sources, and comes with the ~same suite of tests.Source normalisation of plugins is better suited to the marshaling phase than the parsing/unmarshaling phase, because:
pipeline
is spun out into its own library, users would be writingPlugin
literals with all the accepted kinds ofSource
, so normalisation would generally happen later.There's also a couple of log output fumbles (
%w
is only supported infmt.Errorf
, not logger) and a fix to make it easier to holdordered.Unmarshal
when you haveyaml.Node
.