Skip to content

Conversation

@alpinemiaou
Copy link
Contributor

Resolves #893

Description

This replaces Scanner with NewReader able to handle large amount of data returned by helm ouptut when executing diff action.
More info (https://stackoverflow.com/a/29444042)

Changes

  • Changing pkg/helmexec/runner_test.go TestLiveOutput test, adding test with large amount of data, reproducing the issue before applying this fix
  • Changing pkg/helmexec/runner.go Scan with Readstring

Test env

  • kind v0.20.0 go1.20.5 darwin/arm64
  • kindest/node:v1.27.3
  • helm version.BuildInfo{Version:"v3.13.2", GitCommit:"2a2fb3b98829f1e0be6fb18af2f6599e0f4e8243", GitTreeState:"clean", GoVersion:"go1.21.4"}
  • helm diff version 3.6.0

Comments

  • before pkg/helmexec/runner.go changes introduced with this PR, running TestLiveOutput results in timeout:
Running tool: /opt/homebrew/opt/go/libexec/bin/go test -timeout 30s -run ^TestLiveOutput$ github.com/helmfile/helmfile/pkg/helmexec

panic: test timed out after 30s
running tests:
    TestLiveOutput (30s)
    TestLiveOutput/live_output_data (30s)
...

@yxxhero
Copy link
Member

yxxhero commented Nov 11, 2023

@flabatut good solution. thanks so much. BTW please ci DCO issue.

This replaces Scanner with ReadString to handle large amount of data returned by helm ouptut when executing diff action

- Changing pkg/helmexec/runner_test.go TestLiveOutput test, adding test with large amount of data, reproducing the issue before applying this fix
- Changing pkg/helmexec/runner.go Scanner with Readstring

Resolves #893

Signed-off-by: Franck Labatut <franck.labatut@ubisoft.com>
@yxxhero
Copy link
Member

yxxhero commented Nov 11, 2023

@flabatut unit tests failed.

Signed-off-by: Franck Labatut <franck.labatut@ubisoft.com>
@alpinemiaou
Copy link
Contributor Author

@yxxhero , using vscode debugger, I missed the data race detector option.
From now, running make test locally succeeds.
my 2 cents: I guess this is good enough but might be worth looking at https://github.com/go-cmd/cmd to deal with concurrency easily.

@yxxhero yxxhero merged commit b8a729d into helmfile:main Nov 11, 2023
@alpinemiaou alpinemiaou deleted the fix-enable-live-output branch November 12, 2023 06:14
virginiabrioso pushed a commit to virginiabrioso/helmfile that referenced this pull request Nov 30, 2023
* fix: support large output with --enable-live-ouput

This replaces Scanner with ReadString to handle large amount of data returned by helm ouptut when executing diff action

- Changing pkg/helmexec/runner_test.go TestLiveOutput test, adding test with large amount of data, reproducing the issue before applying this fix
- Changing pkg/helmexec/runner.go Scanner with Readstring

Resolves helmfile#893

Signed-off-by: Franck Labatut <franck.labatut@ubisoft.com>

* fix: prevent data race

Signed-off-by: Franck Labatut <franck.labatut@ubisoft.com>

---------

Signed-off-by: Franck Labatut <franck.labatut@ubisoft.com>
Signed-off-by: Virginia Tavares <virginia.tavares@ericsson.com>
JadeHayes pushed a commit to Secretions/helmfile that referenced this pull request Dec 7, 2023
* fix: support large output with --enable-live-ouput

This replaces Scanner with ReadString to handle large amount of data returned by helm ouptut when executing diff action

- Changing pkg/helmexec/runner_test.go TestLiveOutput test, adding test with large amount of data, reproducing the issue before applying this fix
- Changing pkg/helmexec/runner.go Scanner with Readstring

Resolves helmfile#893

Signed-off-by: Franck Labatut <franck.labatut@ubisoft.com>

* fix: prevent data race

Signed-off-by: Franck Labatut <franck.labatut@ubisoft.com>

---------

Signed-off-by: Franck Labatut <franck.labatut@ubisoft.com>
Signed-off-by: Jade Hayes <jade.hayes@dominodatalab.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

helmfile hangs on large diff with --enable-live-output

2 participants