search

renovatebot / renovate

Home of the Renovate CLI: Cross-platform Dependency Automation by Mend.io
https://mend.io/renovate
GNU Affero General Public License v3.0
17.45k stars 2.3k forks source link

Standardize PR artifact error messages due to externally executed commands #16916

Open Gabriel-Ladzaretti opened 2 years ago

Gabriel-Ladzaretti commented 2 years ago

Describe the proposed change(s).

Following -

and @elchenberg's comment.

When artifacts error occurs, the logs might contains important information which are excluded from the corresponding PR.

After inspecting the error handling in different artifacts related sections, it seems that every manager does it differently. some include data from streams and some don't.

We probably should standardize it and include any available data which might help.

examples - mix https://github.com/renovatebot/renovate/blob/dd5ca94a03cdfb5d3db21e66ba0d4f7de2500786/lib/modules/manager/mix/artifacts.ts#L102-L104 https://github.com/renovatebot/renovate/blob/dd5ca94a03cdfb5d3db21e66ba0d4f7de2500786/lib/modules/manager/mix/artifacts.ts#L115-L120

bundler https://github.com/renovatebot/renovate/blob/dd5ca94a03cdfb5d3db21e66ba0d4f7de2500786/lib/modules/manager/bundler/artifacts.ts#L182-L184 https://github.com/renovatebot/renovate/blob/dd5ca94a03cdfb5d3db21e66ba0d4f7de2500786/lib/modules/manager/bundler/artifacts.ts#L203-L215

jb https://github.com/renovatebot/renovate/blob/dd5ca94a03cdfb5d3db21e66ba0d4f7de2500786/lib/modules/manager/jsonnet-bundler/artifacts.ts#L52-L61 https://github.com/renovatebot/renovate/blob/dd5ca94a03cdfb5d3db21e66ba0d4f7de2500786/lib/modules/manager/jsonnet-bundler/artifacts.ts#L103-L111

cocopods https://github.com/renovatebot/renovate/blob/dd5ca94a03cdfb5d3db21e66ba0d4f7de2500786/lib/modules/manager/cocoapods/artifacts.ts#L91-L105

viceice commented 2 years ago

i think with our new exec wrapper we should simply combine stdout and stderr to a single string. that way error Handling is much more easy.

Gabriel-Ladzaretti commented 2 years ago

i think with our new exec wrapper we should simply combine stdout and stderr to a single string. that way error Handling is much more easy.

Thats what i did first actually, but then i thought that this might become too long and we might want to collapse the streams and leave the err.message as is.

but i guess both approaches are good

EDIT: i think i misunderstood what you meant. did you mean to combine them in the ExecError class directly? this might be easier indeed.

EDIT2: but standardization is still needed as some managers return err.message only and some return some other combination of err.stderr/out & err.message

Gabriel-Ladzaretti commented 2 years ago

my first impl was to have the stringifyArtifactError part of the exec-error.ts. i was checking if instance of exec-error if so i did the same as below, otherwise i would just return err.message

https://github.com/renovatebot/renovate/blob/de4563f55f74c2b2cbe6881e073a23df30c168a0/lib/workers/repository/update/branch/index.ts#L703-L713

rarkins commented 2 years ago

We ideally want to just show people the error. The whole stdout could be very large in some cases

Gabriel-Ladzaretti commented 2 years ago

As https://github.com/renovatebot/renovate/pull/16961 is merged, error.message will now include the command ran and stderr, therefore some will have duplicate error messages. if we are ok with it, this can be closed, otherwise we can for example opt for error.message as a standard artifact error message

viceice commented 2 years ago

not all tools are writing errors to stderr

rarkins commented 2 years ago

Our old logic used to be "Include stderr in the PR comment but fall back to ardour if stderr is empty"