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

GitHub Action for plugin install #2239

Merged

Conversation

stephen-crawford
Copy link
Contributor

Description

Converts the security plugin install into a git hub action action.yml process instead of a workflow.

Partially resolves issue #2207

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]
Completed check on 2.4 branch for windows and linux os with jdk 11 and 17.

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.

@stephen-crawford stephen-crawford requested a review from a team November 4, 2022 23:14
@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2022

Codecov Report

Merging #2239 (f1998c0) into main (93fe633) will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #2239      +/-   ##
============================================
+ Coverage     61.02%   61.04%   +0.01%     
- Complexity     3267     3268       +1     
============================================
  Files           259      259              
  Lines         18337    18337              
  Branches       3248     3248              
============================================
+ Hits          11191    11193       +2     
+ Misses         5561     5559       -2     
  Partials       1585     1585              
Impacted Files Coverage Δ
...ch/security/auditlog/routing/AsyncStoragePool.java 55.55% <0.00%> (+5.55%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Great work, lets try to condense some of these steps to have fewer platform specific code paths and fewer inputs, looks really close!

.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
@stephen-crawford
Copy link
Contributor Author

@peternied this should be all set now.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Please add those tests back in, and I think there might be some issues with the workflow setting up the linux instance correctly.

.github/workflows/plugin_install.yml Outdated Show resolved Hide resolved
.github/workflows/plugin_install.yml Outdated Show resolved Hide resolved
.github/workflows/plugin_install.yml Outdated Show resolved Hide resolved
.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
.github/actions/start-opensearch/action.yml Outdated Show resolved Hide resolved
@stephen-crawford
Copy link
Contributor Author

@cwperks @peternied @DarshitChanpura This should be all set.

@peternied
Copy link
Member

This pull request is pointed to the 2.4 branch, but we are past feature freeze. Seems like we should point it to main, what do you think?

@stephen-crawford
Copy link
Contributor Author

This pull request is pointed to the 2.4 branch, but we are past feature freeze. Seems like we should point it to main, what do you think?

Main does not have a stable windows build is the reason this is on 2.4.

@peternied
Copy link
Member

If we can't get it working on main, then we should go into 2.X - do you have a link to the issue about windows being unavailable?

@stephen-crawford
Copy link
Contributor Author

opensearch-project/opensearch-build#2754 At the bottom of the page, it is mentioned that there was not a successful build for 3.0. This may have changed but I have been working on this for a while so I think when I started it was still not there. I can move this up to being based on 3 and see what happens but I am not sure how to confirm one-way or the other whether the main is not a viable base.

@stephen-crawford stephen-crawford force-pushed the ActionForInstall2.4 branch 2 times, most recently from 227ff17 to 4e7bc51 Compare November 10, 2022 14:47
@stephen-crawford stephen-crawford changed the base branch from 2.4 to main November 10, 2022 14:53
@stephen-crawford stephen-crawford changed the title Action for install 2.4 Action for plugin install Nov 10, 2022
@peternied
Copy link
Member

I think you need to rebase your changes with something like git rebase origin/main and then force push to your branch. Checkout the following guide if this is your first time https://www.digitalocean.com/community/tutorials/how-to-rebase-and-update-a-pull-request

@stephen-crawford
Copy link
Contributor Author

I did and resolved all the conflicts. Not sure why it is not running the checks again but you can see it is up-to-date with main. I will try again.

@peternied
Copy link
Member

I don't think the rebase / force push worked. If you look at the pull request there is 125 commits in this change. Maybe squash and force push

@peternied
Copy link
Member

image

I don't see any changes? Just in case I have found this useful too https://ohshitgit.com/

Signed-off-by: Stephen Crawford <steecraw@amazon.com>
@stephen-crawford
Copy link
Contributor Author

image

peternied
peternied previously approved these changes Nov 10, 2022
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Well done!

@stephen-crawford
Copy link
Contributor Author

@cwperks what do you think about merging this?

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Great work on the abstraction of the github action to promote re-usability! Left a few suggestions, and one area that needs to be addressed. The section on integTests is not working, can you look into it?

Signed-off-by: Stephen Crawford <steecraw@amazon.com>
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 force-pushed the ActionForInstall2.4 branch 3 times, most recently from 9cee894 to c477372 Compare November 11, 2022 21:20
@stephen-crawford
Copy link
Contributor Author

I am not sure why this stopped working all of a sudden.

@stephen-crawford
Copy link
Contributor Author

Seems there is something wrong with gradle and not the code so I will reflog my changes and then see where that gets us once it is fixed.

Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Great work, thank you @scrawfor99!

@peternied peternied changed the title Action for plugin install GitHub Action for plugin install Nov 17, 2022
@peternied peternied merged commit e36d396 into opensearch-project:main Nov 17, 2022
@stephen-crawford stephen-crawford deleted the ActionForInstall2.4 branch April 11, 2023 14:02
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.

5 participants