Skip to content

[Backport 1.3] Add plugin install workflow and action #2300

Merged
peternied merged 5 commits intoopensearch-project:1.3from
stephen-crawford:1.3
Dec 6, 2022
Merged

[Backport 1.3] Add plugin install workflow and action #2300
peternied merged 5 commits intoopensearch-project:1.3from
stephen-crawford:1.3

Conversation

@stephen-crawford
Copy link
Contributor

Signed-off-by: Stephen Crawford steecraw@amazon.com

Description

Backport the plugin install workflow and github action to 1.3. The backport got missed when everything else was merged into one branch.

Backports merged PR #2271 to 1.3 branch.

Testing

Tested for functionality on Java and Windows with JDK 8, 11, 14. It is worth noting that the plugin install workflow seems slightly flaky on jdk 11 alone. I encountered this issue first when I was originally merging PR #2271. For whatever reason the setup.bat file does not always echo the 'yes' response into the install script as expected. It passes over 50% of the time though.

Check List

  • New functionality includes testing
  • New functionality has been documented
  • 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.

Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
@stephen-crawford stephen-crawford requested a review from a team December 6, 2022 15:39
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
if: ${{ runner.os == 'Windows' }}
with:
url: https://artifacts.opensearch.org/snapshots/core/opensearch/${{ inputs.opensearch-version }}-SNAPSHOT/opensearch-min-${{ inputs.opensearch-version }}-SNAPSHOT-windows-x64-latest.zip

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please remove this extra line?

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather we don't attempt to review/update style comments in pull requests like number of blank lines. If you'd like to see a type of style enforced - I'd look into a tool that can enforce/correct the style towards a preference.

Checkout actions in the github marketplace [1] that could prettify/lint yml.

[1] https://github.com/marketplace/actions/yaml-lint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I remove it or leave it? I can do either.

if: ${{ runner.os == 'Windows' }}
with:
url: https://artifacts.opensearch.org/snapshots/core/opensearch/${{ inputs.opensearch-version }}-SNAPSHOT/opensearch-min-${{ inputs.opensearch-version }}-SNAPSHOT-windows-x64-latest.zip

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather we don't attempt to review/update style comments in pull requests like number of blank lines. If you'd like to see a type of style enforced - I'd look into a tool that can enforce/correct the style towards a preference.

Checkout actions in the github marketplace [1] that could prettify/lint yml.

[1] https://github.com/marketplace/actions/yaml-lint

@cwperks cwperks changed the title Add plugin install workflow and action [Backport 1.3] Add plugin install workflow and action Dec 6, 2022
@stephen-crawford
Copy link
Contributor Author

@cwperks @peternied can this be merged if we are leaving the line?

@cwperks
Copy link
Member

cwperks commented Dec 6, 2022

Changed the branch protections to now make the plugin-install checks required instead of linux-install and windows-install. I added the requirement only for JDK11, but that can be changed if needed.

@peternied peternied merged commit 64ebd9e into opensearch-project:1.3 Dec 6, 2022
@peternied
Copy link
Member

As soon as the option to merge is green you can merge, no matter any outstanding comments.

Conversely, if there are any changes maintainers would like before a PR is merged they should use 'Request Changes' to ensure that gets resolved before the code goes in.

@cwperks
Copy link
Member

cwperks commented Dec 6, 2022

@scrawfor99 Should this change be backported to the 2.x line as well? I only see it in main and 1.3 now.

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