Add check of component versions to CI #414

dblock · 2021-09-08T20:48:40Z

Description

Introduce checks into CI that verify that the versions of dependent components match OpenSearch, where possible. With this change most components go through CI checks when they get changed in the manifest to ensure that they have the correct versions. For example, this change would have caught #410.

Extracts PropertiesFile which can now be instantiated empty, from a Dict or a file.
Adds checks:
- CheckGradleDependencies: runs gradlew dependencies, parses the tree and loads as PropertiesFile
- CheckGradleDependenciesProjectOpenSearchVersion: checks that opensearch.version matches
- CheckGradleProperties: runs gradlew properties, loads as PropertiesFile
- CheckGradlePropertiesVersion: checks that version matches opensearch version plus .0

A handful of components (alerting, performance-analyzer-rc and security) don't have these checks enabled because they have maven or non-standard gradle commands, and don't respond to things like ./gradlew properties. Will follow-up via #437.

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.

codecov-commenter · 2021-09-08T20:52:23Z

Codecov Report

Merging #414 (b8d4b04) into main (a9da775) will increase coverage by 2.80%.
The diff coverage is 90.25%.

@@            Coverage Diff             @@
##             main     #414      +/-   ##
==========================================
+ Coverage   59.42%   62.23%   +2.80%     
==========================================
  Files          38       45       +7     
  Lines        1119     1234     +115     
==========================================
+ Hits          665      768     +103     
- Misses        454      466      +12

Impacted Files	Coverage Δ
bundle-workflow/src/ci.py	`0.00% <0.00%> (ø)`
bundle-workflow/src/ci_workflow/ci.py	`57.89% <50.00%> (-27.82%)`	⬇️
bundle-workflow/src/git/git_repository.py	`90.69% <50.00%> (-9.31%)`	⬇️
bundle-workflow/src/ci_workflow/check.py	`93.33% <93.33%> (ø)`
bundle-workflow/src/system/properties_file.py	`97.36% <97.36%> (ø)`
...ndle-workflow/src/build_workflow/build_recorder.py	`98.90% <100.00%> (+0.71%)`	⬆️
...kflow/src/ci_workflow/check_gradle_dependencies.py	`100.00% <100.00%> (ø)`
...i_workflow/check_gradle_dependencies_opensearch.py	`100.00% <100.00%> (ø)`
...orkflow/src/ci_workflow/check_gradle_properties.py	`100.00% <100.00%> (ø)`
...src/ci_workflow/check_gradle_properties_version.py	`100.00% <100.00%> (ø)`
... and 10 more

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 a9da775...b8d4b04. Read the comment docs.

Signed-off-by: dblock <dblock@dblock.org>

bundle-workflow/src/ci_workflow/check_gradle_dependencies.py

peternied · 2021-09-09T15:52:23Z

bundle-workflow/src/ci_workflow/check.py

+from abc import ABC, abstractmethod
+
+
+class Check(ABC):


[Nit] This class seems like it is doing double duty, its got a description of a git repo, version and build target information as well as an interface for validation something about the check itself. Could resolve by making it a VersionCheck, or BuildCheck, or breaking them data fields into another object.

Good point. I'll refactor this separately since it's the same pattern in build_workflow and needs to be addressed at the same time.

peternied · 2021-09-09T15:53:18Z

bundle-workflow/src/ci_workflow/check_gradle_dependencies.py

+        self.dependencies = self.__get_dependencies()
+
+    def __get_dependencies(self):
+        cmd = " ".join(


[Optional] Seems like there is shared constructions between CheckGradleProperties. Might be useful to centralize concepts around gradle in one place such as having GradleCmd(self, target=':dependencies') vs GradleCmd(self, target='properties') where self is a Check or VersionCheck subclass

I suppose if we do more gradle commands for sure.

peternied · 2021-09-09T15:58:00Z

bundle-workflow/src/ci_workflow/check.py

+
+    @abstractmethod
+    def check(self):
+        logging.info(f"Checking {self.component.name}, {self.__class__.__name__}.")


[Nit] Reshape, rename to __perform_check is abstract does the checking, add concrete check that does logging around the __perform_check call for consistency.

Good suggestion, but I actually don't want this logging most of the time, this method should just be a pass.

mch2 · 2021-09-09T16:46:26Z

bundle-workflow/src/ci_workflow/check_gradle_publish_to_maven_local.py

+    def check(self):
+        cmd = " ".join(
+            [
+                "./gradlew publishToMavenLocal",


I understand right now only two components will use this check, but this feels like we are getting closer and closer to simply running a build with
./bundle-workflow/build.sh manifests/opensearch-1.1.0.yml.

Each component that uses this is essentially doing a full build of that component.

What if we just run the full build if there is a change to a manifest file?
Something like.

on: pull_request: paths: - manifests/*.yml

You have a point, however this is actually quite far from a full build, only some gradle runs to extract properties. Once we have maven we don't need to publish OpenSearch and common-utils to maven local, so this CI will be much faster.

If we start building everything we're going to start running out of space on workers, it will take 2 hours on GHA, etc.

I added running CI on manifests only in PRs, that's a good idea.

mch2 · 2021-09-09T17:19:29Z

.github/workflows/manifests.yml

@@ -22,11 +22,17 @@ jobs:
    runs-on: ubuntu-latest
    env:
      PYTHON_VERSION: 3.7
+      JDK_VERSION: 14


If we are not backporting java 11 to 1.0 branch, can we start with 11 here and make a separate job for the 1.0 manifests?

Everything is still on 14.

Signed-off-by: dblock <dblock@dblock.org>

saratvemulapalli

I took one pass at it.
Couple of questions, overall it looks good to me.

saratvemulapalli · 2021-09-10T16:14:12Z

DEVELOPER_GUIDE.md

@@ -141,7 +141,7 @@ Run from `bundle-workflow` before making pull requests.
 cd bundle-workflow

 pipenv run isort .
-pipenv run black .
+git status -s | grep -e "[MA?]\s.*.py" | cut -c4- | xargs pipenv run black


This is cryptic...

Trust me. It just picks all .py files that are added or modified, minus the deleted ones. It's a dev readme anyway, so you're supposed to parse regexp in your head ;)

I opened #442 to add a pre-commit hook.

saratvemulapalli · 2021-09-10T16:50:34Z

bundle-workflow/src/ci_workflow/check_gradle_dependencies.py

+                f"./gradlew {self.gradle_project or ''}:dependencies",
+                f"-Dopensearch.version={self.opensearch_version}",
+                f"-Dbuild.snapshot={str(self.snapshot).lower()}",
+                '| grep -e "---"',


Just trying to understand:
Looks like we are only checking opensearch and its version. Why not just look for compile time dependencies instead of all of them?

Also for next steps, we should verify plugin dependencies on other plugins and make sure their versions also match the manifest.

Do you know how to get just those? I can open a separate PR for improving this.

#440 and #441

Yeah a simple way is to do something like ./gradlew :dependencies --configuration compile

Eg: From our fav plugin AD

~anomaly-detection$ ./gradlew :dependencies --configuration compile > Configure project : ======================================= OpenSearch Build Hamster says Hello! Gradle Version : 6.6.1 OS Info : Linux 5.4.0-1037-aws (amd64) JDK Version : 14 (JDK) JAVA_HOME : /usr/lib/jvm/java-14-openjdk-amd64 Random Testing Seed : DFA8FDF8CA53A245 In FIPS 140 mode : false ======================================= > Task :dependencies ------------------------------------------------------------ Root project - OpenSearch anomaly detector plugin ------------------------------------------------------------ compile - Dependencies for source set 'main' (deprecated, use 'implementation' instead). (n) +--- org.opensearch:opensearch:1.0.0-rc1 (n) +--- org.opensearch:common-utils:1.0.0.0-rc1 (n) +--- com.google.guava:guava:29.0-jre (n) +--- org.apache.commons:commons-math3:3.6.1 (n) +--- com.google.code.gson:gson:2.8.6 (n) +--- com.yahoo.datasketches:sketches-core:0.13.4 (n) +--- com.yahoo.datasketches:memory:0.12.2 (n) +--- commons-lang:commons-lang:2.6 (n) +--- software.amazon.randomcutforest:randomcutforest-core:1.0 (n) +--- software.amazon.randomcutforest:randomcutforest-serialization-json:1.0 (n) +--- org.opensearch.client:opensearch-rest-client:1.0.0-rc1 (n) +--- org.jacoco:org.jacoco.agent:0.8.5 (n) \--- org.jacoco:org.jacoco.ant:0.8.5 (n)

saratvemulapalli

Looks good to me! Thanks @dblock !!

dblock force-pushed the ci-gradle-versions branch 2 times, most recently from 19019f2 to 6348c55 Compare September 8, 2021 20:50

dblock force-pushed the ci-gradle-versions branch 3 times, most recently from 269fe3f to 20d3a74 Compare September 8, 2021 21:23

dblock added 2 commits September 9, 2021 12:08

Check output of gradle properties and dependencies.

139837d

Signed-off-by: dblock <dblock@dblock.org>

Build prerequisites until we have Maven.

e3df59b

Signed-off-by: dblock <dblock@dblock.org>

dblock force-pushed the ci-gradle-versions branch from 20d3a74 to 4d26e0f Compare September 9, 2021 12:09

Build publish to maven local as a check.

b6f6167

Signed-off-by: dblock <dblock@dblock.org>

dblock force-pushed the ci-gradle-versions branch from 4d26e0f to 146c11d Compare September 9, 2021 14:57

Unit tests.

bf10dc5

Signed-off-by: dblock <dblock@dblock.org>

dblock force-pushed the ci-gradle-versions branch from 146c11d to bf10dc5 Compare September 9, 2021 15:01

Fix: comment.

1d9cda7

Signed-off-by: dblock <dblock@dblock.org>

dblock marked this pull request as ready for review September 9, 2021 15:15

dblock requested review from peternied and mch2 September 9, 2021 15:15

Improve documentation.

b8d4b04

Signed-off-by: dblock <dblock@dblock.org>

dblock force-pushed the ci-gradle-versions branch from 80f1279 to b8d4b04 Compare September 9, 2021 15:51

peternied previously approved these changes Sep 9, 2021

View reviewed changes

VachaShah dismissed peternied’s stale review via b8d4b04 September 9, 2021 16:24

mch2 previously approved these changes Sep 9, 2021

View reviewed changes

dblock added 2 commits September 9, 2021 17:36

Remove unnecessary logging.

7a9ff50

Signed-off-by: dblock <dblock@dblock.org>

Added comment.

9acea33

Signed-off-by: dblock <dblock@dblock.org>

dblock dismissed mch2’s stale review via 9acea33 September 9, 2021 17:40

peternied previously approved these changes Sep 9, 2021

View reviewed changes

Only run CI on manifest changes.

5d3e0cf

Signed-off-by: dblock <dblock@dblock.org>

dblock dismissed peternied’s stale review via 5d3e0cf September 9, 2021 18:40

dblock requested review from mch2 and peternied September 9, 2021 18:55

peternied approved these changes Sep 9, 2021

View reviewed changes

dblock requested a review from setiah September 10, 2021 11:31

This was referenced Sep 10, 2021

[BUG] Missing CI checks for various components #437

Closed

Use common-utils 1.1 branch for OpenSearch 1.1 #430

Merged

dblock requested a review from saratvemulapalli September 10, 2021 15:45

saratvemulapalli reviewed Sep 10, 2021

View reviewed changes

This was referenced Sep 10, 2021

When verifying dependencies in CI, only look for compile time dependencies #440

Closed

Verify dependencies between plugins in CI #441

Open

Add a precommit hook for linting python code #442

Closed

dblock requested a review from saratvemulapalli September 10, 2021 17:28

saratvemulapalli approved these changes Sep 10, 2021

View reviewed changes

dblock merged commit e60a664 into opensearch-project:main Sep 10, 2021

dblock deleted the ci-gradle-versions branch September 10, 2021 17:37

dblock mentioned this pull request Sep 15, 2021

Use compileOnly configuration when checking dependency versions, faster. #495

Merged

1 task

jordarlu mentioned this pull request Jul 10, 2023

Add placeholder plugins when auto-generating next version's manifests #3707

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check of component versions to CI #414

Add check of component versions to CI #414

dblock commented Sep 8, 2021 •

edited

Loading

codecov-commenter commented Sep 8, 2021 •

edited

Loading

peternied Sep 9, 2021

dblock Sep 9, 2021

peternied Sep 9, 2021

dblock Sep 9, 2021

peternied Sep 9, 2021

dblock Sep 9, 2021 •

edited

Loading

mch2 Sep 9, 2021

dblock Sep 9, 2021

dblock Sep 9, 2021

mch2 Sep 9, 2021

dblock Sep 9, 2021 •

edited

Loading

saratvemulapalli left a comment

saratvemulapalli Sep 10, 2021

dblock Sep 10, 2021 •

edited

Loading

dblock Sep 10, 2021

saratvemulapalli Sep 10, 2021

dblock Sep 10, 2021

dblock Sep 10, 2021

saratvemulapalli Sep 10, 2021

saratvemulapalli left a comment •

edited

Loading

Add check of component versions to CI #414

Add check of component versions to CI #414

Conversation

dblock commented Sep 8, 2021 • edited Loading

Description

Check List

codecov-commenter commented Sep 8, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

saratvemulapalli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saratvemulapalli left a comment • edited Loading

Choose a reason for hiding this comment

dblock commented Sep 8, 2021 •

edited

Loading

codecov-commenter commented Sep 8, 2021 •

edited

Loading

dblock Sep 9, 2021 •

edited

Loading

dblock Sep 9, 2021 •

edited

Loading

dblock Sep 10, 2021 •

edited

Loading

saratvemulapalli left a comment •

edited

Loading