Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

retain the error when appending the stderr output #3219

Merged
merged 1 commit into from
Aug 19, 2020
Merged

retain the error when appending the stderr output #3219

merged 1 commit into from
Aug 19, 2020

Conversation

dvulpe
Copy link
Contributor

@dvulpe dvulpe commented Jul 27, 2020

This PR retains the Go error in sync.go:603 which should help debugging why flux fails synchronising in #3217

@stefanprodan stefanprodan requested a review from squaremo July 28, 2020 14:25
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @dvulpe

@acondrat
Copy link

acondrat commented Aug 6, 2020

We seem to have the same problem and no clue to what is causing it. Any chance to see this merged/released?
Thanks!

@stefanprodan
Copy link
Member

/rebase

@squaremo
Copy link
Member

squaremo commented Aug 6, 2020

Isn't the err from err := cmd.Run() just going to be command exited with 1?

@dvulpe
Copy link
Contributor Author

dvulpe commented Aug 6, 2020

For the vast majority of failures you’re right. The go docs mention “other error types may be returned for other situations” as well and we need as much information as possible to understand what’s causing the flakiness.

@dvulpe
Copy link
Contributor Author

dvulpe commented Aug 18, 2020

Apologies for chasing, but we’re still seeing this flakiness in a number of fluxes and we’d like to know if there is any chance for this PR to be accepted.

@squaremo
Copy link
Member

/rebase

@squaremo
Copy link
Member

I beg your pardon, @dvulpe, this got crowded out by a flurry of other things going on leading up to KubeCon. I'll merge it ASAP and put it in the next release.

@squaremo squaremo merged commit 644e381 into fluxcd:master Aug 19, 2020
@dvulpe dvulpe deleted the fix_error_wrap branch August 19, 2020 11:36
@dvulpe
Copy link
Contributor Author

dvulpe commented Aug 19, 2020

No worries, @squaremo - I understand how busy things can get.
I just wanted to be sure there wasn't anything else required for it to be accepted.

Thank you for merging it 🙂

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.

4 participants