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

Fix signature mismatches when steps have plugins #2319

Merged
merged 4 commits into from
Aug 24, 2023

Conversation

DrJosh9000
Copy link
Contributor

@DrJosh9000 DrJosh9000 commented Aug 23, 2023

The first part of this PR enables CommandStep to be unmarshaled directly from JSON consistently. This is mainly needed because Plugin 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:

  • interpolation applies to plugin sources and configs, so putting it in the parsing phase would require reworking interpolation
  • if/when pipeline is spun out into its own library, users would be writing Plugin literals with all the accepted kinds of Source, so normalisation would generally happen later.

There's also a couple of log output fumbles (%w is only supported in fmt.Errorf, not logger) and a fix to make it easier to hold ordered.Unmarshal when you have yaml.Node.

@DrJosh9000 DrJosh9000 force-pushed the pdp-1133-normalise-plugins branch 2 times, most recently from 84f9f02 to 71229a9 Compare August 24, 2023 04:02
@DrJosh9000 DrJosh9000 marked this pull request as ready for review August 24, 2023 04:13
@DrJosh9000 DrJosh9000 changed the title WIP: Fix signature mismatches when steps have plugins Fix signature mismatches when steps have plugins Aug 24, 2023
@DrJosh9000 DrJosh9000 force-pushed the pdp-1133-normalise-plugins branch 2 times, most recently from fbc488e to c428fdd Compare August 24, 2023 05:03
@DrJosh9000 DrJosh9000 requested a review from a team August 24, 2023 05:11
@DrJosh9000 DrJosh9000 force-pushed the pdp-1133-normalise-plugins branch from c428fdd to ab13e10 Compare August 24, 2023 05:20
@DrJosh9000 DrJosh9000 force-pushed the pdp-1133-normalise-plugins branch from 78aaaa5 to ed7d9ee Compare August 24, 2023 05:46
@DrJosh9000 DrJosh9000 force-pushed the pdp-1133-normalise-plugins branch from ed7d9ee to 948bba6 Compare August 24, 2023 06:02
Copy link
Contributor

@triarius triarius left a 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
Copy link
Contributor

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.

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 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)
Copy link
Contributor

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

@DrJosh9000 DrJosh9000 merged commit 3b58d0e into main Aug 24, 2023
@DrJosh9000 DrJosh9000 deleted the pdp-1133-normalise-plugins branch August 24, 2023 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants