fix: performance issues yq #261 #288

Dunky13 · 2021-01-22T10:35:48Z

Using jq for faster recursive key deletion.

Using jq for faster recursive key deletion. fix #261

bin/bootstrap.sh

0-sv

Awesome that the performance is improved now. It might be a "nice to have" and not a "must have" for this change, but since the generate_loose_schema function is a module in the bin/bootstrap.sh script (ie. it can be tested by bats by importing the function), you could add a test to bin/tests with an example sourcePath and targetPath testing this function.

Removed "unneeded chars" #288 #261

Write a bats test to check whether yq & jq work correctly. #261 #288

…-perf-bats

Morriz

please ditch the "tests" that were just added

Morriz · 2021-01-29T11:01:26Z

bin/tests/bootstrap.bats

+    result=$(yq r "$PWD/values-schema.yaml" '**.required.' | wc -l)
+    [ "$result" -ne 0 ]
+    result=$(yq r "$PWD/values-schema.yaml" 'properties.toolsVersion' | wc -l)
+    [ "$result" -ne 0 ]
+    result=$(yq r "$PWD/values-schema.yaml" 'properties.cluster' | wc -l)
+    [ "$result" -ne 0 ]
+
+    result=$(yq r "$ENV_DIR/.vscode/values-schema.yaml" '**.required.' | wc -l)
+    [ "$result" -eq 0 ]
+    result=$(yq r "$ENV_DIR/.vscode/values-schema.yaml" 'properties.toolsVersion' | wc -l)
+    [ "$result" -eq 0 ]
+    result=$(yq r "$ENV_DIR/.vscode/values-schema.yaml" 'properties.cluster' | wc -l)
+    [ "$result" -eq 0 ]


Why are we suddenly addings tests that only test the workings of yq here? Can we discuss these things first? This is undesirable

@Morriz was discussed as desirable during standup.
It is not testing the workings of yq, but verifying that the original yaml contains these keys, and the new yaml does not contain these keys.
So it verifies that the (now speed optimized) removes these fields correctly.

It is testing nothing of our code. It is merely testing if yq does what you ask it to do. Call me if you want further clarification.

@Morriz we could've discussed this during stand-up. It can't be that you have to micro-manage every piece of code, it's like you don't trust anyone.

I'm just going to lay out how I experienced it. First you create an issue that the performance of the bats testing is not good. That's true, it takes about 30 seconds. Of course there's yq to blame, but it's not nice to reproach me about code you added. Then when I mention the performance gain of piping to jq, you don't believe me.

Now this test will actually test if the behaviour is the same of pre-jq and post-jq. It's about 6 lines of code and to me it seems like this would improve coverage because the logic is actually in the function generate_loose_schema. If someone adds a different piece of logic, say, we don't need to remove the required keys anymore (I don't actually understand why those need to be removed in the first place, since it's valid json schema), then this test will complain and that makes it easier to debug.

Of course, if there is anything wrong about my reasoning then I'm open to learn from you, as always.

Removed a couple of "configurable" options and made them hard coded as well as fixing a couple of bash issues like `"$@"` #261 #288

Dunky13

@Morriz why use the v for version?

charts/team-ns/values.yaml

values/otomi-api/otomi-api.gotmpl

Dunky13 · 2021-01-29T15:37:53Z

@Morriz could it be that since you had me remove the "toolsVersion" definition in value-schema.yaml that since there is still a "toolsVersion" key/value in core.yaml and chars/team-ns/values.yaml that the validate-values complains?

I don't know how to proceed to fix this and have CI stop complaining. And it also seems out-of-scope for this issue/task. Should we revert & recreate in separate issue?

Morriz · 2021-01-29T15:46:05Z

It is about relationship with a version the context expects. Indeed it can happen that all contexts demand an upgrade (like for kubectl minor bumps), but a one for all approach could introduce a breaking change that breaks a context demanding the old image. Maybe I see this wrong but now we have control over that. If we can avoid such breakages and instead use one for all that would be great. But we have to rely on code for that, not human process. I don’t see that go away easily.

0-sv · 2021-02-01T12:57:35Z

It is about relationship with a version the context expects. Indeed it can happen that all contexts demand an upgrade (like for kubectl minor bumps), but a one for all approach could introduce a breaking change that breaks a context demanding the old image. Maybe I see this wrong but now we have control over that. If we can avoid such breakages and instead use one for all that would be great. But we have to rely on code for that, not human process. I don’t see that go away easily.

I can agree with this proposition, and I definitely see the benefits. Of course, I could also argue that detecting breaking changes should be part of our CI.

Besides that, I don't see why this should be part of this task, it seems like something we should discuss. I hope we can enter a discourse on this topic.

Morriz · 2021-02-02T16:53:17Z

@Dunky13 can you resolve these conflicts? You can ask help from me tomorrow if needed...

Dunky13 · 2021-02-02T20:38:33Z

@Dunky13 can you resolve these conflicts? You can ask help from me tomorrow if needed...

I would love to, but have not been able to resolve this (yet). Help is necessary

0-sv · 2021-02-03T09:23:08Z

@Morriz but can we please stay within the issue of improving yq performance?

Had some merge conflicts when merging from master. Resolved in this commit #288

@j-zimnowoda

Discussed with @j-zimnowoda t oadd this docs line #288

@j-zimnowoda

The issue was that the schema did not contain the toolsVersion, and therefor threw an error. As suggested by @j-zimnowoda the toolsVersion is added back, with a readOnly tag and a description. If this is not present, the merged values from the env.gotmpl will fail during testing. #288

j-zimnowoda · 2021-02-03T10:26:32Z

@Morriz could it be that since you had me remove the "toolsVersion" definition in value-schema.yaml that since there is still a "toolsVersion" key/value in core.yaml and chars/team-ns/values.yaml that the validate-values complains?

I don't know how to proceed to fix this and have CI stop complaining. And it also seems out-of-scope for this issue/task. Should we revert & recreate in separate issue?

@Morriz, adding toolsVersion value in core.yaml file and not defining this property in values-schema.yaml causes validation error. Before requesting changes and enforcing them, please think about how does it work. Otherwise you are providing very confusing requirements that people cannot meet.

Morriz · 2021-02-03T13:57:53Z

Then that was caught by our update. Great! So it should be removed from the values.

Morriz

small potatoes left

charts/team-ns/values.yaml

Morriz · 2021-02-03T14:02:03Z

core.yaml

@@ -1,3 +1,4 @@
+# NOTE: This file is merged with other values in ./helmfile.d/snippets/env.gotmpl
 toolsVersion: 1.4.9


this line has to go

That line was suggested by @j-zimnowoda

yes, I suggested it because it is not obvious that core.yaml is part of merged values.
Our colleagues spend a significant time on finding out the root cause of the validation issue, because they did not know about that. I think it is worth to have this command if it can save someone's time in the future.

docs/BATS.md

values-schema.yaml

Removing toolsVersion from all yaml files, as well as updating the BATS docs #288

Morriz

YAY!

fix: rewrite slow yq command using jq

cb52eef

Using jq for faster recursive key deletion. fix #261

Dunky13 requested review from 0-sv and Morriz January 22, 2021 10:35

Dunky13 self-assigned this Jan 22, 2021

Morriz suggested changes Jan 22, 2021

View reviewed changes

bin/bootstrap.sh Outdated Show resolved Hide resolved

0-sv reviewed Jan 22, 2021

View reviewed changes

Marc Went and others added 4 commits January 22, 2021 14:44

fix: removed "unneeded chars"

54510ba

Removed "unneeded chars" #288 #261

Merge branch 'master' into yq-perf-bats

485ef8f

fix: write bats test for generate_loose_schema

9d0986e

Write a bats test to check whether yq & jq work correctly. #261 #288

Merge branch 'yq-perf-bats' of github.com:redkubes/otomi-core into yq…

d29368c

…-perf-bats

Morriz suggested changes Jan 29, 2021

View reviewed changes

fix: together with @Morriz "cleaned up" the codebase

8d1f97a

Removed a couple of "configurable" options and made them hard coded as well as fixing a couple of bash issues like `"$@"` #261 #288

Dunky13 commented Jan 29, 2021

View reviewed changes

charts/team-ns/values.yaml Outdated Show resolved Hide resolved

0-sv reviewed Jan 29, 2021

View reviewed changes

values/otomi-api/otomi-api.gotmpl Show resolved Hide resolved

Marc Went added 3 commits February 3, 2021 11:16

fix: merge master fixes

d1c3732

Had some merge conflicts when merging from master. Resolved in this commit #288

docs: added small note to inform where this file is used

b56a819

Discussed with @j-zimnowoda t oadd this docs line #288

Merge branch 'master' into yq-perf-bats

a343f55

Morriz suggested changes Feb 3, 2021

View reviewed changes

fix: resolving small potatoes

f89e400

Removing toolsVersion from all yaml files, as well as updating the BATS docs #288

Morriz self-requested a review February 4, 2021 17:08

Morriz approved these changes Feb 4, 2021

View reviewed changes

Morriz merged commit a98e1c8 into master Feb 4, 2021

delete-merged-branch bot deleted the yq-perf-bats branch February 4, 2021 17:12

Dunky13 mentioned this pull request Mar 31, 2021

Create npm task to transform values-schema from jsonschema to OAS 3.1 spec linode/apl-api#151

Closed

Dunky13 added a commit that referenced this pull request May 27, 2021

fix: performance issues yq #261 (#288)

a52892b

Ani1357 pushed a commit that referenced this pull request Oct 25, 2023

fix: performance issues yq #261 (#288)

8e2da6e

		@@ -1,3 +1,4 @@
		# NOTE: This file is merged with other values in ./helmfile.d/snippets/env.gotmpl
		toolsVersion: 1.4.9

fix: performance issues yq #261 #288

fix: performance issues yq #261 #288

Uh oh!

Conversation

Dunky13 commented Jan 22, 2021

Uh oh!

Uh oh!

0-sv left a comment

Choose a reason for hiding this comment

Uh oh!

Morriz left a comment

Choose a reason for hiding this comment

Uh oh!

Morriz Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

Dunky13 Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

Morriz Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

0-sv Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

Dunky13 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Dunky13 commented Jan 29, 2021

Uh oh!

Morriz commented Jan 29, 2021 via email

Uh oh!

0-sv commented Feb 1, 2021

Uh oh!

Morriz commented Feb 2, 2021

Uh oh!

Dunky13 commented Feb 2, 2021

Uh oh!

0-sv commented Feb 3, 2021

Uh oh!

j-zimnowoda commented Feb 3, 2021

Uh oh!

Morriz commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Morriz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Morriz Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

Dunky13 Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

j-zimnowoda Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Morriz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Morriz commented Feb 3, 2021 •

edited

Loading