[WIP] Refractor the check-for-build method and tests

[zelinh]

Description

I found out a problem regarding of the PR just merged #1628. When we disable propagate when trigger the job, the following

echo "Build succeeded, uploading build SHA for that job"
buildUploadManifestSHA(
    inputManifest: "manifests/${INPUT_MANIFEST}",
    jobName: "${TARGET_JOB_NAME}"
)

will be executed after triggered job failed which not what we expected. We shouldn't upload it if the distribution-build failed.

The use of try-catch block will avoid this situation, if the distribution-build failed, nothing below the job call in the try block will be executed.

Issues Resolved

continue on #1627

Check List

• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov-commenter]

Codecov Report

Merging #1630 (6fa31fb) into main (e0a34eb) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1630   +/-   ##
=========================================
  Coverage     94.67%   94.67%           
  Complexity       13       13           
=========================================
  Files           163      163           
  Lines          3437     3437           
  Branches         21       21           
=========================================
  Hits           3254     3254           
  Misses          180      180           
  Partials          3        3           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0a34eb...6fa31fb. Read the comment docs.

[dblock]

Should this be closed in favor of #1628?

[peternied]

I think that miss on he previous PR could have been caught by unit tests. Before we do more work here can we add tests for this jenkinsfile that cover the success and failure build cases?

Note; we should revert the previous PR if this is impacting the builds negatively

[dblock]

I think that miss on he previous PR could have been caught by unit tests. Before we do more work here can we add tests for this jenkinsfile that cover the success and failure build cases?

Note; we should revert the previous PR if this is impacting the builds negatively

Oh my bad, read too quickly. Sounds like the SHA should be uploaded inside the build job instead?

[zelinh]

I think that miss on he previous PR could have been caught by unit tests. Before we do more work here can we add tests for this jenkinsfile that cover the success and failure build cases?
Note; we should revert the previous PR if this is impacting the builds negatively

Oh my bad, read too quickly. Sounds like the SHA should be uploaded inside the build job instead?

Yes, that's another way I was considering.

[zelinh]

I think that miss on he previous PR could have been caught by unit tests. Before we do more work here can we add tests for this jenkinsfile that cover the success and failure build cases?

Note; we should revert the previous PR if this is impacting the builds negatively

Yes, adding a test for check-for-build is definitely needed as we have a meta issue here. #1468; while that's not in the scope of this PR. This PR provides an approach for resolving #1627 and moving SHA upload to build could also be another approach but it will require to be added in both distribution-build-opensearch & distribution-build-opensearch-dashboards. WDYT? @dblock

[dblock]

I think that miss on he previous PR could have been caught by unit tests. Before we do more work here can we add tests for this jenkinsfile that cover the success and failure build cases?
Note; we should revert the previous PR if this is impacting the builds negatively

Yes, adding a test for check-for-build is definitely needed as we have a meta issue here. #1468; while that's not in the scope of this PR. This PR provides an approach for resolving #1627 and moving SHA upload to build could also be another approach but it will require to be added in both distribution-build-opensearch & distribution-build-opensearch-dashboards. WDYT? @dblock

You can't use try/catch because it will swallow the error from uploading the SHA, and we'll never know. I would move it.

[zelinh]

I think that miss on he previous PR could have been caught by unit tests. Before we do more work here can we add tests for this jenkinsfile that cover the success and failure build cases?
Note; we should revert the previous PR if this is impacting the builds negatively

Yes, adding a test for check-for-build is definitely needed as we have a meta issue here. #1468; while that's not in the scope of this PR. This PR provides an approach for resolving #1627 and moving SHA upload to build could also be another approach but it will require to be added in both distribution-build-opensearch & distribution-build-opensearch-dashboards. WDYT? @dblock

You can't use try/catch because it will swallow the error from uploading the SHA, and we'll never know. I would move it.

That makes sense.. Working on it.

[peternied]

We should still add tests to confirm the behavior is correct

[zelinh]

Closing this for now since this is not being worked actively.