Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TEST_MANIFEST param to check-for-build jenkins job #1585

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Feb 2, 2022

Signed-off-by: Tyler Ohlsen [email protected]

Description

Currently distribution builds are broken due to a missing TEST_MANIFEST parameter in the check-for-build jenkins job, which was introduced in the distribution-build-opensearch job as part of #1517. This PR adds that.

Note that only distribution-build-opensearch is using a test manifest, so there's separate logic for the distribution-build-opensearch-dashboards job to exclude that parameter for know. In the future these may get merged back, once Dashboards supports integ tests and using a test manifest.

Issues Resolved

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.

Copy link
Member

@gaiksaya gaiksaya left a comment

Choose a reason for hiding this comment

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

Wondering if we can pass null or none instead of if else block. won't matter as param will not be used anywhere and will be consistent

@peternied peternied merged commit 484f6bc into opensearch-project:main Feb 3, 2022
@ohltyler
Copy link
Member Author

ohltyler commented Feb 3, 2022

@gaiksaya we can, but then it will show up in Jenkins UI with an extra TEST_MANIFEST parameter in the build info, and may add confusion since it's not being used. Prefer to leave as an if/else for now and merge together again once Dashboards supports a test manifest similar to core OpenSearch.

build job: "${TARGET_JOB_NAME}", parameters: [
string(name: 'INPUT_MANIFEST', value: "${INPUT_MANIFEST}")
], wait: true
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a hack, but at least it should have else error, otherwise if this is ever passed to a job that's not distribution-build-... there's no failure and nothing is kicked off.

Then, does passing TEST_MANIFEST into distribution-build-opensearch-dashboards that doesn't expect it actually fails?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, there should be an else branch - this was reviewed and merged in a short time. I'll follow up on that.

Regarding passing TEST_MANIFEST, it won't fail but could add confusion, see comment above.

Copy link
Member

Choose a reason for hiding this comment

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

It's NBD, but maybe a comment would suffice.

peterzhuamazon pushed a commit to peterzhuamazon/opensearch-build that referenced this pull request Feb 16, 2022
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