Updates to gradle build file #48

sruti1312 · 2021-08-19T01:45:56Z

Signed-off-by: Sruti Parthiban partsrut@amazon.com

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

Describe the solution you are proposing
Introduced two different env variables that will be used for building RCA package (which runs as a part of PA build).
cloneRcaProject -> this is used to clone the main branch of the RCA package. If set true, it clones RCA package into ../performance-analyzer-rca location . By default it is set to false in that case it checks the same location for this package, if it exists the test continues. If not, the test fails.
rcaProjectDir -> this is used to specify the location where the rca package is checked out. It defaults to ../performance-analyzer-rca

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
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: Sruti Parthiban <partsrut@amazon.com>

dblock · 2021-08-19T13:34:05Z

build.gradle

@@ -69,6 +70,15 @@ spotbugsTest {
 ext {
    opensearchVersion = '1.0.0'
    isSnapshot = "true" == System.getProperty("build.snapshot", "true")
+
+    // Name of the default git branch. This is used for checking out the RCA repository
+    gitDefaultBranch = 'main'


Is there an explanation somewhere why this is needed?

This should probably be parameterized, e.g. -Dperformance-analyzer-rca.branch=main and I would rename cloneRcaProject to -Dperformance-analyzer-rca.build=false or something like that to be consistent.

As a developer I probably also want to be able to -Dperformance-analyzer-rca.path=... instead, but see my meta question of trying to understand why this dependency is managed differently from other types of dependencies.

+1
If RCA is always built as part of the assembled plugin zip for performance-analyzer, we will need the ability to define the branch that is needed for a particular release. This works fine with main but will be needed to correctly reproduce builds from tags/branches from both repos.

Alternative - It sounds like the RCA zip should be included as a maven publication that is fetched and used to build the final bundle in this repo rather than cloning & rebuilding or looking on a local path.

I have modified the parameter names and added an additional parameter performance-analyzer-rca.branch that helps to specify the RCA branch for checkout (defaults to main).

Answering to why this is different from other types of dependencies. Currently, for creating the PA artifact, we rely on some files and folders from the RCA package, we use gradle for building and bring in these RCA bits into PA artifact.
opendistro-for-elasticsearch/performance-analyzer#131

RCA package is more of a component built and shipped along with PA plugin than a dependency.

dblock · 2021-08-19T13:38:51Z

I am confused why the whole "build performance analyzer RCA" is done inside a gradle build. Plugins have dependencies such as, say common-utils. They require that the latter be built and published into maven (local). You can do it in a CI workflow right now, and in some not so distant future pull those dependencies from Maven. Why is this any different that requires one to check out the source as part of the build and build it?

mch2

This is causing the assembled zip to be missing RCA - #49

codecov-commenter · 2021-08-19T22:22:49Z

Codecov Report

Merging #48 (96ecbf6) into main (75340a4) will not change coverage.
The diff coverage is n/a.

❗ Current head 96ecbf6 differs from pull request most recent head 4bd15eb. Consider uploading reports for the commit 4bd15eb to get more accurate results

@@            Coverage Diff            @@
##               main      #48   +/-   ##
=========================================
  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 75340a4...4bd15eb. Read the comment docs.

Signed-off-by: Sruti Parthiban <partsrut@amazon.com>

sruti1312 · 2021-08-19T23:00:29Z

This is causing the assembled zip to be missing RCA - #49

Fixed this issue, its working as expected now.

dblock

This works.

We probably do agree that building another project inline is not the way to go in the long term? The RCA project needs to build and publish itself in a way that can be picked up by PA, just like all the dependencies. @sruti1312, since you understand the history for this, could you please open an issue for fixing this with maybe some ideas about how we should be solving it for the long term, and link it here?

sruti1312 · 2021-08-20T00:24:59Z

#50

Created an issue to track to remove building RCA package during PA build and consuming it like a dependency.

* Updates to gradle build file Signed-off-by: Sruti Parthiban <partsrut@amazon.com> * Add ability to specify RCA branch Signed-off-by: Sruti Parthiban <partsrut@amazon.com>

* Create writer file if metrics are available (#31) Signed-off-by: Sruti Parthiban <partsrut@amazon.com> * Add tests to check for writer file only if metrics are present (#35) Signed-off-by: Sruti Parthiban <partsrut@amazon.com> * Merge pull request #36 from opensearch-project/khushbr-writer-purge-fix Fixing Event Log file cleanup issue * Moving deleteAllFiles() to inside scheduleExecutor() * Fixing the Link Checker errors, updating the official documentation * nit: Fixing spotlessJava indentation issue * Merge pull request #37 from khushbr/feature/purge-fix Handling purging of lingering files before scheduleExecutor start. * Fix failing file handler test (#38) Signed-off-by: Sruti Parthiban <partsrut@amazon.com> * Remove dependency on main branch when running spotless. (#47) Signed-off-by: Marc Handalian <handalm@amazon.com> * Updates to gradle build file (#48) * Updates to gradle build file Signed-off-by: Sruti Parthiban <partsrut@amazon.com> * Add ability to specify RCA branch Signed-off-by: Sruti Parthiban <partsrut@amazon.com> * Fix build when opensearch_version flag is provided. (#52) Signed-off-by: Marc Handalian <handalm@amazon.com> * Update the version to 1.0.1 Signed-off-by: Sruti Parthiban <partsrut@amazon.com> Co-authored-by: Khushboo Rajput <59671881+khushbr@users.noreply.github.com> Co-authored-by: Khushboo Rajput <khushbr@amazon.com> Co-authored-by: Marc Handalian <handalm@amazon.com>

Updates to gradle build file

ece31ee

Signed-off-by: Sruti Parthiban <partsrut@amazon.com>

sruti1312 requested a review from kjoseph07 August 19, 2021 01:46

mch2 approved these changes Aug 19, 2021

View reviewed changes

dblock reviewed Aug 19, 2021

View reviewed changes

mch2 suggested changes Aug 19, 2021

View reviewed changes

Add ability to specify RCA branch

4bd15eb

Signed-off-by: Sruti Parthiban <partsrut@amazon.com>

sruti1312 force-pushed the feature/build-gradle-update branch from 2b3bd95 to 4bd15eb Compare August 19, 2021 22:27

sruti1312 requested a review from dblock August 19, 2021 23:03

dblock approved these changes Aug 19, 2021

View reviewed changes

mch2 approved these changes Aug 19, 2021

View reviewed changes

sruti1312 merged commit 83aafb1 into main Aug 20, 2021

sruti1312 deleted the feature/build-gradle-update branch August 20, 2021 00:25

sruti1312 mentioned this pull request Aug 24, 2021

Updates to gradle build file and GitHub workflow #5

Closed

5 tasks

sruti1312 added a commit that referenced this pull request Aug 25, 2021

Updates to gradle build file (#48)

4d242db

* Updates to gradle build file Signed-off-by: Sruti Parthiban <partsrut@amazon.com> * Add ability to specify RCA branch Signed-off-by: Sruti Parthiban <partsrut@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to gradle build file #48

Updates to gradle build file #48

sruti1312 commented Aug 19, 2021 •

edited

Loading

dblock Aug 19, 2021 •

edited

Loading

mch2 Aug 19, 2021

mch2 Aug 19, 2021

sruti1312 Aug 19, 2021 •

edited

Loading

dblock commented Aug 19, 2021 •

edited

Loading

mch2 left a comment

codecov-commenter commented Aug 19, 2021 •

edited

Loading

sruti1312 commented Aug 19, 2021

dblock left a comment •

edited

Loading

sruti1312 commented Aug 20, 2021

Updates to gradle build file #48

Updates to gradle build file #48

Conversation

sruti1312 commented Aug 19, 2021 • edited Loading

Check List

dblock Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

mch2 Aug 19, 2021

Choose a reason for hiding this comment

mch2 Aug 19, 2021

Choose a reason for hiding this comment

sruti1312 Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

dblock commented Aug 19, 2021 • edited Loading

mch2 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 19, 2021 • edited Loading

Codecov Report

sruti1312 commented Aug 19, 2021

dblock left a comment • edited Loading

Choose a reason for hiding this comment

sruti1312 commented Aug 20, 2021

sruti1312 commented Aug 19, 2021 •

edited

Loading

dblock Aug 19, 2021 •

edited

Loading

sruti1312 Aug 19, 2021 •

edited

Loading

dblock commented Aug 19, 2021 •

edited

Loading

codecov-commenter commented Aug 19, 2021 •

edited

Loading

dblock left a comment •

edited

Loading