Only re-encode subst value if it smells like json and target doc is yaml - Githubissues

open-component-model / ocm

Open Component Model (Software Bill of Delivery Toolset)

https://ocm.software

Apache License 2.0

31 stars 19 forks source link

Only re-encode subst value if it smells like json and target doc is yaml #785

Closed dee0sap closed 1 month ago

dee0sap commented 2 months ago

Description

Currently we always re-encode subst value in an effort to have its encoding ( json or yaml ) match that of the target document. More specifically we are trying to avoid having something that looks like a yaml doc with json mixed into it as this is off putting.

The problems with this are

More complicated code as we need more error checks in place for the marshall and unmarshall calls that always take place
At least as implemented, if the value is actually yaml we may lose some formatting and we will lose tags and anchors

With this PR we only perform the re-encoding if the target is not json and if the subst value 'smells' like json.

What type of PR is this? (check all applicable)

[ ] 🍕 Feature
[ ] 🎇 Restructuring
[ ] 🐛 Bug Fix
[ ] 📝 Documentation Update
[ ] 🎨 Style
[ ] 🧑‍💻 Code Refactor
[X ] 🔥 Performance Improvements
[X ] ✅ Test
[ ] 🤖 Build
[ ] 🔁 CI
[ ] 📦 Chore (Release)
[ ] ⏩ Revert

Related Tickets & Documents

Please see discussion between myself and @Skarlso

Screenshots

Added tests?

[x ] 👍 yes
[ ] 🙅 no, because they aren't needed
[ ] 🙋 no, because I need help
[ ] Separate ticket for tests # (issue/pr)

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added to documentation?

[ ] 📜 README.md
[x ] 🙅 no documentation needed

Checklist:

[ x] My code follows the style guidelines of this project
[ x] I have performed a self-review of my code
[ x] I have commented my code, particularly in hard-to-understand areas
[ x] I have made corresponding changes to the documentation
[ x] My changes generate no new warnings
[ x] I have added tests that prove my fix is effective or that my feature works
[ x] New and existing unit tests pass locally with my changes
[ x] Any dependent changes have been merged and published in downstream modules

Skarlso commented 1 month ago

Running the tests for this pr.

Skarlso commented 1 month ago

make test
/Users/skarlso/goprojects/SAP/ocm-controller/bin/controller-gen rbac:roleName=ocm-controller-manager-role crd webhook paths="./api/..." paths="./controllers/..." output:crd:artifacts:config=config/crd/bases
/Users/skarlso/goprojects/SAP/ocm-controller/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./api/..." paths="./controllers/..."
go fmt ./...
go vet ./...
tinygo build -target=wasi -panic=trap -scheduler=none -no-debug -o ./internal/wasm/hostfuncs/resource/testdata/get_resource_bytes.wasm  ./internal/wasm/hostfuncs/resource/testdata/get_resource_bytes.go
tinygo build -target=wasi -panic=trap -scheduler=none -no-debug -o ./internal/wasm/hostfuncs/resource/testdata/get_resource_labels.wasm  ./internal/wasm/hostfuncs/resource/testdata/get_resource_labels.go
tinygo build -target=wasi -panic=trap -scheduler=none -no-debug -o ./internal/wasm/hostfuncs/resource/testdata/get_resource_url.wasm  ./internal/wasm/hostfuncs/resource/testdata/get_resource_url.go
KUBEBUILDER_ASSETS="/Users/skarlso/Library/Application Support/io.kubebuilder.envtest/k8s/1.24.1-darwin-amd64" go test ./... -coverprofile cover.out
        github.com/open-component-model/ocm-controller          coverage: 0.0% of statements
        github.com/open-component-model/ocm-controller/api/v1alpha1             coverage: 0.0% of statements
?       github.com/open-component-model/ocm-controller/internal/wasm/hostfuncs/types    [no test files]
?       github.com/open-component-model/ocm-controller/pkg/configdata   [no test files]
?       github.com/open-component-model/ocm-controller/pkg/cache        [no test files]
        github.com/open-component-model/ocm-controller/internal/wasm/hostfuncs          coverage: 0.0% of statements
        github.com/open-component-model/ocm-controller/pkg/metrics              coverage: 0.0% of statements
        github.com/open-component-model/ocm-controller/internal/wasm/hostfuncs/logging          coverage: 0.0% of statements
        github.com/open-component-model/ocm-controller/internal/wasm/io         coverage: 0.0% of statements
        github.com/open-component-model/ocm-controller/pkg/fakes                coverage: 0.0% of statements
        github.com/open-component-model/ocm-controller/pkg/cache/fakes          coverage: 0.0% of statements
ok      github.com/open-component-model/ocm-controller/controllers      7.269s  coverage: 45.7% of statements
ok      github.com/open-component-model/ocm-controller/internal/wasm/hostfuncs/resource 4.683s  coverage: 45.8% of statements
ok      github.com/open-component-model/ocm-controller/pkg/component    2.663s  coverage: 5.6% of statements
ok      github.com/open-component-model/ocm-controller/pkg/event        1.544s  coverage: 2.7% of statements
?       github.com/open-component-model/ocm-controller/pkg/version      [no test files]
        github.com/open-component-model/ocm-controller/pkg/status               coverage: 0.0% of statements
        github.com/open-component-model/ocm-controller/pkg/wasm/runtime         coverage: 0.0% of statements
        github.com/open-component-model/ocm-controller/pkg/wasm/errors          coverage: 0.0% of statements
        github.com/open-component-model/ocm-controller/pkg/ocm/fakes            coverage: 0.0% of statements
        github.com/open-component-model/ocm-controller/pkg/snapshot             coverage: 0.0% of statements
        github.com/open-component-model/ocm-controller/pkg/wasm/config          coverage: 0.0% of statements
        github.com/open-component-model/ocm-controller/pkg/version/generate             coverage: 0.0% of statements
ok      github.com/open-component-model/ocm-controller/pkg/oci  1.650s  coverage: 17.8% of statements
ok      github.com/open-component-model/ocm-controller/pkg/ocm  3.066s  coverage: 28.1% of statements

All tests passed with this pr.

Skarlso commented 1 month ago

@dee0sap Can you please sign your commits? Then we can merge this. :)

dee0sap commented 1 month ago

@dee0sap Can you please sign your commits? Then we can merge this. :)

Done I had to reset the branch however as I made a mistake when rebasing. Nothing broken but the merges performed by you won't be in the history anymore

Skarlso commented 1 month ago

@dee0sap You will need this pr https://github.com/open-component-model/ocm/pull/791 to fix the lint issue once it's merged and has gone green. 🤞

Skarlso commented 1 month ago

Oh f*ck. The linter update, of course, will kill everything.

Skarlso commented 1 month ago

Sorry, I'm going to bed, we'll have to deal with those tomorrow. Sorry, Dan. :(