Skip to content

Conversation

@LukasHeimann
Copy link
Contributor

@LukasHeimann LukasHeimann commented Mar 28, 2025

In my project, i try to use the packages setting to build multiple binaries at the same time:

  golangBuild:
    output: i2-automated-pipelines-test
    privateModules: "github.tools.sap/*"
    packages:
      - ./cmd/run1
      - ./cmd/run2

This triggers a code path leading to isMainPackage. Unfortunately, inside all of this, the go binary fails with exit code 1:

info  golangBuild - running command: go list -f {{ .Name }} ./cmd/run1
fatal golangBuild - execution of golang build failed - failed to calculate output binaries or directory, error: running command 'go' failed: cmd.Run() failed: exit status 1

It must be exactly the go command inside isMainPackage, because in the loop to get the output paths, no other go binary is executed, and my second package (run2) is not logged.

When I run the command go list -f "{{ .Name }}" ./cmd/run1 locally, I don't get any error, the command outputs main instead -- which is the non failure path.

Unfortunately, when the go binary fails in this case, we don't see any error output, as console output is entirely redirected to outBuffer, which is not returned.

This PR improves this by using fmt.Errorf to attach the outBuffer to the error. Using %w to wrap the original error, any introspection using errors.Is and errors.As would still work (although noone seems to do this). I also made sure to only add the additional context of the error output after the original error, so the current error message stays unchanged -- there is just more context at the end of it.

Please consider merging this change to improve the error output.

In my project, i try to use the `packages` setting to build multiple binaries at the same time, which triggers the code path leading to `isMainPackage`. Unfortunately, inside all of this, the go binary fails with exit code 1:

```
 info  golangBuild - running command: go list -f {{ .Name }} ./cmd/run1
fatal golangBuild - execution of golang build failed - failed to calculate output binaries or directory, error: running command 'go' failed: cmd.Run() failed: exit status 1
```

It must be exactly the go command inside isMainPackage, because in the loop to get the output paths, no other go binary is executed, and my second package (run2) is not logged.

When I run the command `go list -f "{{ .Name }}" ./cmd/run1` locally, I don't get any error, the command outputs `main` instead -- which is the non failure path.

Unfortunately, when the go binary fails in this case, we don't see any error output, as console output is entirely redirected to `outBuffer`, which is not returned.

This PR improves this by using `fmt.Errorf` to attach the outBuffer to the error. Using `%w` to wrap the original error, any introspection using `errors.Is` and `errors.As` would still work (although noone seems to do this). I also made sure to only add the additional context of the error output _after_ the original error, so the current error message stays unchanged -- there is just more context at the end of it.

Please consider merging this change to improve the error output.
@LukasHeimann LukasHeimann requested a review from a team as a code owner March 28, 2025 11:52
@LukasHeimann LukasHeimann changed the title [golangBuild] Add better error logging to isMainPackage [golangBuild] Add better error logging to isMainPackage Mar 28, 2025
@valfz
Copy link
Contributor

valfz commented Mar 28, 2025

/it-go

@ashlymat
Copy link
Member

ashlymat commented Apr 7, 2025

@LukasHeimann Thanks for the PR.
Is it possible to add a small unit test?

@LukasHeimann
Copy link
Contributor Author

LukasHeimann commented Apr 7, 2025

@ashlymat Done! I hope, this is what you expected -- a small test specifically targeting the (changed) error message.

Not sure why the sonar bot is not picking up the new coverage, though -- I very explicitly test the string concatenation handling in the changed function...

@Googlom
Copy link
Member

Googlom commented Apr 28, 2025

/it-go

@sonarqubecloud
Copy link

@Googlom Googlom enabled auto-merge (squash) April 28, 2025 07:54
@Googlom Googlom merged commit 9d1dc17 into SAP:master Apr 28, 2025
13 checks passed
maxatsap added a commit to maxatsap/jenkins-library that referenced this pull request May 8, 2025
* origin: (27 commits)
  fix(golang): handle errors with simple module names (SAP#5339)
  Revert "fix(integration): Skip failing intergration tests for now (SAP#5… (SAP#5330)
  Abap env cf cli latest (SAP#5338)
  [golangBuild] Add better error logging to `isMainPackage` (SAP#5308)
  abapEnvironment steps cf-cli v12 to latest (SAP#5333)
  Add link to CNB docs for run images (SAP#5336)
  feat(detect): add flag containerScan to detect step (SAP#5312)
  fix(sonarExecuteScan): update sonarScannerDownloadUrl (SAP#5314)
  (improvement!) handle case if AdoPersonalAccessToken is not set (SAP#5322)
  fix(gcs): return Temporary hold error without retry (SAP#5335)
  feat(cnbBuild): enable additional syft catalogers for sbom generation (SAP#5332)
  chore(deps): Update direct dependencies (SAP#5327)
  fix(integration): Skip failing intergration tests for now (SAP#5329)
  Update CODEOWNERS for Sonar files (SAP#5317)
  chore(sonar): drop code ownership for sonar (SAP#5315)
  chore(): Replaced node version `lts-buster` with `lts-bookworm` (SAP#5286)
  feat: output hooks values in stage-config.json (SAP#5311)
  fix(docs): update Gauge url (SAP#5313)
  [ABAP] Add type to log output (SAP#5310)
  usage of BASH from PATH (SAP#5282)
  ...
maxatsap added a commit to maxatsap/jenkins-library that referenced this pull request May 8, 2025
* origin: (27 commits)
  fix(golang): handle errors with simple module names (SAP#5339)
  Revert "fix(integration): Skip failing intergration tests for now (SAP#5… (SAP#5330)
  Abap env cf cli latest (SAP#5338)
  [golangBuild] Add better error logging to `isMainPackage` (SAP#5308)
  abapEnvironment steps cf-cli v12 to latest (SAP#5333)
  Add link to CNB docs for run images (SAP#5336)
  feat(detect): add flag containerScan to detect step (SAP#5312)
  fix(sonarExecuteScan): update sonarScannerDownloadUrl (SAP#5314)
  (improvement!) handle case if AdoPersonalAccessToken is not set (SAP#5322)
  fix(gcs): return Temporary hold error without retry (SAP#5335)
  feat(cnbBuild): enable additional syft catalogers for sbom generation (SAP#5332)
  chore(deps): Update direct dependencies (SAP#5327)
  fix(integration): Skip failing intergration tests for now (SAP#5329)
  Update CODEOWNERS for Sonar files (SAP#5317)
  chore(sonar): drop code ownership for sonar (SAP#5315)
  chore(): Replaced node version `lts-buster` with `lts-bookworm` (SAP#5286)
  feat: output hooks values in stage-config.json (SAP#5311)
  fix(docs): update Gauge url (SAP#5313)
  [ABAP] Add type to log output (SAP#5310)
  usage of BASH from PATH (SAP#5282)
  ...
maxatsap added a commit to maxatsap/jenkins-library that referenced this pull request May 8, 2025
* origin: (27 commits)
  fix(golang): handle errors with simple module names (SAP#5339)
  Revert "fix(integration): Skip failing intergration tests for now (SAP#5… (SAP#5330)
  Abap env cf cli latest (SAP#5338)
  [golangBuild] Add better error logging to `isMainPackage` (SAP#5308)
  abapEnvironment steps cf-cli v12 to latest (SAP#5333)
  Add link to CNB docs for run images (SAP#5336)
  feat(detect): add flag containerScan to detect step (SAP#5312)
  fix(sonarExecuteScan): update sonarScannerDownloadUrl (SAP#5314)
  (improvement!) handle case if AdoPersonalAccessToken is not set (SAP#5322)
  fix(gcs): return Temporary hold error without retry (SAP#5335)
  feat(cnbBuild): enable additional syft catalogers for sbom generation (SAP#5332)
  chore(deps): Update direct dependencies (SAP#5327)
  fix(integration): Skip failing intergration tests for now (SAP#5329)
  Update CODEOWNERS for Sonar files (SAP#5317)
  chore(sonar): drop code ownership for sonar (SAP#5315)
  chore(): Replaced node version `lts-buster` with `lts-bookworm` (SAP#5286)
  feat: output hooks values in stage-config.json (SAP#5311)
  fix(docs): update Gauge url (SAP#5313)
  [ABAP] Add type to log output (SAP#5310)
  usage of BASH from PATH (SAP#5282)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants