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.

Sign up for GitHub

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

Jump to bottom

Fix snapshot build using OpenSearch 1.1. #53

Merged
merged 3 commits into from
Aug 23, 2021
Merged

Fix snapshot build using OpenSearch 1.1. #53

merged 3 commits into from
Aug 23, 2021

Conversation

dblock
Copy link
Member

@dblock dblock commented Aug 20, 2021 •
edited
Loading

Signed-off-by: dblock [email protected]

Is your feature request related to a problem? Please provide an existing Issue # , or describe.

  • Fix snapshot build.
  • Use OpenSearch 1.1 by default.

There's a circular dependency described in #50, so opensearch-project/performance-analyzer-rca#55 is needed. Once this is merged I'll switch references to my fork that contains this code.

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.

@dblock dblock mentioned this pull request Aug 20, 2021
Closed
14 tasks
@dblock dblock force-pushed the fix-snapshot-build branch 4 times, most recently from 14d2ec8 to 6bb5f77 Compare August 20, 2021 20:54
@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2021 •
edited
Loading

Codecov Report

Merging #53 (5f46ab5) into main (bcdbc22) will not change coverage.
The diff coverage is n/a.

❗ Current head 5f46ab5 differs from pull request most recent head 7550cc9. Consider uploading reports for the commit 7550cc9 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##               main      #53   +/-   ##
=========================================
  Coverage     72.18%   72.18%           
  Complexity      356      356           
=========================================
  Files            44       44           
  Lines          2509     2509           
  Branches        160      160           
=========================================
  Hits           1811     1811           
  Misses          592      592           
  Partials        106      106

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 bcdbc22...7550cc9. Read the comment docs.

@dblock dblock mentioned this pull request Aug 20, 2021
Merged
1 task
@dblock dblock requested a review from sruti1312 August 20, 2021 21:53
sruti1312
sruti1312 reviewed Aug 20, 2021
View reviewed changes
build.gradle Outdated
}
exec {
def licenseDir = "$projectDir/licenses"
workingDir("$licenseDir")
commandLine 'rm', "-f", "performanceanalyzer-rca-1.0.0.0.jar.sha1"
commandLine 'rm', "-f", "performanceanalyzer-rca-${project.version}.jar.sha1"
Copy link
Contributor

@sruti1312 sruti1312 Aug 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

version or opensearch.version?

.github/workflows/gradle.yml
working-directory: ./tmp/performance-analyzer
run: |
./gradlew build -Dperformance-analyzer-rca.build=true \
-Dperformance-analyzer-rca.repo="https://github.com/dblock/performance-analyzer-rca.git" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to the main repo before merging?

Copy link
Member Author

@dblock dblock Aug 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, but it will break the build. So, we can't. And if I merge opensearch-project/performance-analyzer-rca#55 it's the same problem for that PR. I think the safe thing to do is to merge this PR and opensearch-project/performance-analyzer-rca#55 and then go and change on both sides carefully (which I will do).

@dblock dblock merged commit 4e205e4 into opensearch-project:main Aug 23, 2021
This was referenced Aug 23, 2021
Merged
Closed
Merged
@sruti1312 sruti1312 mentioned this pull request Aug 24, 2021
Closed
5 tasks
sruti1312 pushed a commit that referenced this pull request Aug 25, 2021
@dblock @sruti1312
Fix snapshot build using OpenSearch 1.1. (#53)
8f78c80
sruti1312 added a commit that referenced this pull request Aug 25, 2021
@sruti1312 @dblock
Add additional commits from main and upgrade to 1.0.1 (#57)
6417b33 
* Revert back 1.0.1 upgrade

* Fix snapshot build using OpenSearch 1.1. (#53)

* Use performance-analyzer-rca main branch. (#54)

Signed-off-by: dblock <[email protected]>

* Upgrade to 1.0.1 version

Signed-off-by: Sruti Parthiban <[email protected]>

Co-authored-by: Daniel Doubrovkine (dB.) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sruti1312 sruti1312 sruti1312 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dblock @codecov-commenter @sruti1312