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

chore(yaml): add go-yaml types package #1225

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

chore(yaml): add go-yaml types package #1225

wants to merge 11 commits into from

Conversation

ecrupper
Copy link
Contributor

@ecrupper ecrupper commented Dec 3, 2024

I am breaking up #1220 into more digestible PRs.

This PR simply adds the new go-yaml v3 library types package: compiler/types/yaml/yaml. All unmarshaled buildkite YAML gets converted to that type since they are identical post-parsing.

In other words, there is no functional change in this PR. But it sets the stage much better for future changes to make those PRs easier to understand.

@ecrupper ecrupper requested a review from a team as a code owner December 3, 2024 19:00
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 70.61366% with 340 lines in your changes missing coverage. Please review.

Project coverage is 56.63%. Comparing base (f6dd71e) to head (0f94592).

Files with missing lines Patch % Lines
compiler/types/yaml/buildkite/secret.go 72.72% 49 Missing and 2 partials ⚠️
compiler/types/yaml/buildkite/step.go 65.04% 35 Missing and 1 partial ⚠️
compiler/types/yaml/buildkite/template.go 43.75% 36 Missing ⚠️
compiler/types/yaml/buildkite/deployment.go 42.10% 33 Missing ⚠️
compiler/types/yaml/buildkite/service.go 65.90% 28 Missing and 2 partials ⚠️
compiler/types/yaml/buildkite/ruleset.go 81.81% 28 Missing ⚠️
compiler/types/yaml/buildkite/stage.go 19.23% 21 Missing ⚠️
compiler/types/yaml/buildkite/build.go 75.30% 18 Missing and 2 partials ⚠️
compiler/types/yaml/buildkite/metadata.go 66.10% 20 Missing ⚠️
compiler/types/yaml/buildkite/ulimit.go 80.61% 19 Missing ⚠️
... and 4 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1225      +/-   ##
==========================================
+ Coverage   56.19%   56.63%   +0.43%     
==========================================
  Files         609      622      +13     
  Lines       34124    35252    +1128     
==========================================
+ Hits        19176    19964     +788     
- Misses      14284    14616     +332     
- Partials      664      672       +8     
Files with missing lines Coverage Δ
api/pipeline/template.go 0.00% <ø> (ø)
api/types/string.go 80.64% <ø> (ø)
compiler/native/clone.go 100.00% <ø> (ø)
compiler/native/compile.go 69.21% <ø> (ø)
compiler/native/environment.go 87.22% <ø> (ø)
compiler/native/expand.go 67.68% <ø> (ø)
compiler/native/initialize.go 100.00% <ø> (ø)
compiler/native/parse.go 85.36% <100.00%> (ø)
compiler/native/script.go 95.23% <ø> (ø)
compiler/native/substitute.go 70.00% <ø> (ø)
... and 34 more

plyr4
plyr4 previously approved these changes Dec 18, 2024
wass3r
wass3r previously approved these changes Dec 19, 2024
Copy link
Collaborator

@wass3r wass3r left a comment

Choose a reason for hiding this comment

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

i'll reapprove when conflicts are solved

@ecrupper ecrupper dismissed stale reviews from wass3r and plyr4 via 4590305 December 19, 2024 16:24
wass3r
wass3r previously approved these changes Dec 19, 2024
@wass3r wass3r dismissed their stale review December 19, 2024 16:46

failing test

@wass3r
Copy link
Collaborator

wass3r commented Dec 19, 2024

test failed on schema validator 🤔 i confirmed it works fine on main branch. but if you run command manually, you can see the actual error (probably something to improve in the actions workflow).

schema schema.json: ok

instance schema/testdata/pipeline/pass/complex.yml: failed
jsonschema validation failed with 'file:///home/name/ghq/github.com/go-vela/server/schema.json#'
- at '/stages': got object, want array

so, the generated schema differs. i confirmed locally:
❯ diff schema_old.json schema.json

3c3
<   "$id": "https://github.com/go-vela/server/compiler/types/yaml/build",
---
>   "$id": "https://github.com/go-vela/server/compiler/types/yaml/yaml/build",
798,801c798,799
<       "patternProperties": {
<         ".*": {
<           "$ref": "#/$defs/Stage"
<         }
---
>       "items": {
>         "$ref": "#/$defs/Stage"
803,804c801
<       "additionalProperties": false,
<       "type": "object"
---
>       "type": "array"

probably due to missing struct methods like

func (StageSlice) JSONSchemaExtend(schema *jsonschema.Schema) {
. need to port them to new lib? (see #1219 for ref)

Copy link
Collaborator

@wass3r wass3r left a comment

Choose a reason for hiding this comment

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

looks good!

just wanted to point out that none of the json schema methods moved with yaml/buildkite except for

func (StageSlice) JSONSchemaExtend(schema *jsonschema.Schema) {
- not an issue per se, just odd. we're moving away from buildkite, so no big deal. the methods are in yaml/yaml which is where we want them :)

Copy link
Contributor

@KellyMerrick KellyMerrick left a comment

Choose a reason for hiding this comment

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

lgtm

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.

4 participants