-
Notifications
You must be signed in to change notification settings - Fork 277
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
Add Validation Framework #2725
Add Validation Framework #2725
Changes from 3 commits
b9b597c
6fdf7c3
9011dc0
7db7795
d7a055b
34a5f9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
# Copyright OpenSearch Contributors | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
|
||
import logging | ||
import sys | ||
|
||
from system import console | ||
from validation_workflow.validation_args import ValidationArgs | ||
|
||
|
||
def main() -> int: | ||
|
||
args = ValidationArgs() | ||
|
||
console.configure(level=args.logging_level) | ||
logging.getLogger("urllib3").setLevel(logging.WARNING) | ||
|
||
dist = args.DISTRIBUTION_MAP.get(args.distribution, None) | ||
dist.download_artifacts(projects=args.projects, version=args.version, platform=args.platform, architectures=args.SUPPORTED_ARCHITECTURES) | ||
|
||
return 0 | ||
|
||
|
||
if __name__ == "__main__": | ||
sys.exit(main()) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
#### Automate the Validation during Releases. | ||
The input requires a mandatory version number , optional --distribution types(tar,rpm,yum) and --platform type (currently uses only linux) to automatically download and verify the artifacts. | ||
|
||
*Usage* | ||
``` | ||
./validation.sh 2.3.0 --distribution rpm --platform linux | ||
``` | ||
The following options are available. | ||
|
||
| name | description | | ||
|------------------------|---------------------------------------------------------------------| | ||
| version | Accepts a mandatory version number. | | ||
| -d, --distribution | Assigns the distribution type specifed by the user | | ||
| -p, --platform | Determines the platform type of the atrifacts. | | ||
| --verbose | Show more verbose output. | | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
# Copyright OpenSearch Contributors | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
# | ||
# This page intentionally left blank. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
# Copyright OpenSearch Contributors | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
|
||
|
||
import requests | ||
|
||
from system.temporary_directory import TemporaryDirectory | ||
|
||
|
||
class DownloadUtils: | ||
@staticmethod | ||
def is_url_valid(url: str) -> int: | ||
response = requests.head(url) | ||
if response.status_code == 200: | ||
return 1 | ||
else: | ||
return 0 | ||
|
||
@staticmethod | ||
def download(url: str, tmp_dir: TemporaryDirectory) -> bool: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: rename to download_artifacts There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a different function in downloadutils.py which is called by the download_artifacts method in tar, rpm and yum. |
||
# This method writes the contents from the response object into temporary directory file name fetched from the end of the url. | ||
response = requests.get(url, stream=True) | ||
path = tmp_dir.name + "/" + url.split("/")[-1] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not a good implementation and I would rather use os.join because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, if you want to get the file name, use something like os.basename or similar, do not try to split the url and get the last item. |
||
status = bool(open(path, "wb").write(response.content)) | ||
return status |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
# Copyright OpenSearch Contributors | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
|
||
|
||
from abc import ABC, abstractmethod | ||
|
||
|
||
class Validation(ABC): | ||
base_url = "https://artifacts.opensearch.org/releases/bundle/" | ||
|
||
def __init__(self, version: str, distribution: str, platform: str, projects: list) -> None: | ||
self.version = version | ||
self.distribution = distribution | ||
self.platform = platform | ||
self. projects = projects | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo, which would cause the code not running at all. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also means you are missing test cases because the test pass but this code is invalid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't used the init method in base class for implementation.So tried removing the init as I don't need any class properties in particular. |
||
|
||
@classmethod | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this a classmethod after all? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing class method threw me an error |
||
@abstractmethod | ||
def download_artifacts(self, projects: list, version: str, platform: str, architectures: list) -> bool: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Greetings, @Divyaasm , I am working on a dependant issue/PR to validation docker artifact and want to update you that I will need only the 'version' argument to Pull and Spin-up containers by docker-compose; hence, if you can make other arguments in your script as optional, that will help! Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 I believe we can make platform, architectures optional |
||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
# Copyright OpenSearch Contributors | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
|
||
import argparse | ||
import logging | ||
|
||
from validation_workflow.validation_rpm import ValidationRpm | ||
from validation_workflow.validation_tar import ValidationTar | ||
from validation_workflow.validation_yum import ValidationYum | ||
|
||
|
||
class ValidationArgs: | ||
SUPPORTED_DISTRIBUTIONS = [ | ||
"tar", | ||
"yum", | ||
"rpm" | ||
] | ||
SUPPORTED_PLATFORMS = ["linux"] | ||
SUPPORTED_ARCHITECTURES = [ | ||
"x64", | ||
"arm64", | ||
] | ||
DISTRIBUTION_MAP = { | ||
"tar": ValidationTar, | ||
"yum": ValidationYum, | ||
"rpm": ValidationRpm | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should not need one list and one map just to rewrite all the dist again. The design can be using 1 map max: Example:
Then the platform/arch information can be distributed directly in the validation<> class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure Peter, I will use a single dictionary for storing distributions and mapping the distributions to respective classes. Could you please explain what |
||
|
||
version: str | ||
|
||
def __init__(self) -> None: | ||
parser = argparse.ArgumentParser(description="Validation Framework for Validation Workflow.") | ||
parser.add_argument( | ||
"version", | ||
type=str, | ||
help="Product version to validate" | ||
) | ||
parser.add_argument( | ||
"-d", | ||
"--distribution", | ||
type=str, | ||
choices=self.SUPPORTED_DISTRIBUTIONS, | ||
help="Distribution to validate.", | ||
default="tar", | ||
dest="distribution" | ||
) | ||
parser.add_argument( | ||
"-p", | ||
"--platform", | ||
type=str, | ||
choices=self.SUPPORTED_PLATFORMS, | ||
help="Platform to validate.", | ||
default="linux" | ||
) | ||
parser.add_argument( | ||
"-v", | ||
"--verbose", | ||
help="Show more verbose output.", | ||
action="store_const", | ||
default=logging.INFO, | ||
const=logging.DEBUG, | ||
dest="logging_level", | ||
) | ||
|
||
args = parser.parse_args() | ||
self.version = args.version | ||
self.logging_level = args.logging_level | ||
self.distribution = args.distribution | ||
self.platform = args.platform | ||
self.projects = ["opensearch", "opensearch-dashboards"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
# Copyright OpenSearch Contributors | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
|
||
import logging | ||
|
||
from system.temporary_directory import TemporaryDirectory | ||
from validation_workflow.download_utils import DownloadUtils | ||
from validation_workflow.validation import Validation | ||
|
||
|
||
class ValidationRpm(Validation, DownloadUtils): | ||
tmp_dir = TemporaryDirectory() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is a temp dir that all the test requires, why not just put it back to the parent class? |
||
|
||
@classmethod | ||
def download_artifacts(self, projects: list, version: str, platform: str, architectures: list) -> bool: | ||
for project in projects: | ||
for architecture in architectures: | ||
url = f"{self.base_url}{project}/{version}/{project}-{version}-{platform}-{architecture}.rpm" | ||
if ValidationRpm.is_url_valid(url) and ValidationRpm.download(url, self.tmp_dir): | ||
logging.info(f" Valid URL - {url} and Download Successful !") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is there a leading space? |
||
else: | ||
logging.info(f"Invalid URL - {url}") | ||
raise Exception(f"Invalid url - {url}") | ||
return True |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
# Copyright OpenSearch Contributors | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
|
||
import logging | ||
|
||
from system.temporary_directory import TemporaryDirectory | ||
from validation_workflow.download_utils import DownloadUtils | ||
from validation_workflow.validation import Validation | ||
|
||
|
||
class ValidationTar(Validation, DownloadUtils): | ||
tmp_dir = TemporaryDirectory() | ||
|
||
@classmethod | ||
def download_artifacts(self, projects: list, version: str, platform: str, architectures: list) -> bool: | ||
for project in projects: | ||
for architecture in architectures: | ||
url = f"{self.base_url}{project}/{version}/{project}-{version}-{platform}-{architecture}.tar.gz" | ||
if ValidationTar.is_url_valid(url) and ValidationTar.download(url, self.tmp_dir): | ||
logging.info(f" Valid URL - {url} and Download Successful !") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of these implementation are extremely similar, means the core code should exist in parent, and child should just pass the different information. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was initially suggested to have only abstract methods in the base class so I have implemented it this way. I can actually change the core code to base class and pass the required information from the child class. Can I know what you would recommend! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm expecting to leave the method abstract in base class as Docker also extends the same method which has a different implementation all together.So, had to implement it independently for each distribution even though the code is reused. |
||
else: | ||
logging.info(f"Invalid URL - {url}") | ||
raise Exception(f"Invalid url - {url}") | ||
return True |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# Copyright OpenSearch Contributors | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
|
||
import logging | ||
|
||
from system.temporary_directory import TemporaryDirectory | ||
from validation_workflow.download_utils import DownloadUtils | ||
from validation_workflow.validation import Validation | ||
|
||
|
||
class ValidationYum(Validation, DownloadUtils): | ||
tmp_dir = TemporaryDirectory() | ||
|
||
@classmethod | ||
def download_artifacts(self, projects: list, version: str, platform: str, architectures: list) -> bool: | ||
for project in projects: | ||
url = f"{self.base_url}{project}/{version[0:1]}.x/{project}-{version[0:1]}.x.repo" | ||
if ValidationYum.is_url_valid(url) and ValidationYum.download(url, self.tmp_dir): | ||
logging.info(f" Valid URL - {url} and Download Successful !") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. |
||
else: | ||
logging.info(f"Invalid URL - {url}") | ||
raise Exception(f"Invalid url - {url}") | ||
return True |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
# Copyright OpenSearch Contributors | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
|
||
|
||
import unittest | ||
from typing import Any | ||
from unittest.mock import Mock, patch | ||
|
||
import pytest | ||
from pytest import CaptureFixture | ||
|
||
from src.run_validation import main | ||
from src.validation_workflow.validation_args import ValidationArgs | ||
|
||
|
||
class TestRunValidation(unittest.TestCase): | ||
|
||
@pytest.fixture(autouse=True) | ||
def getCapfd(self, capfd: CaptureFixture) -> None: | ||
self.capfd = capfd | ||
|
||
@patch("argparse._sys.argv", ["run_validation.py", "--help"]) | ||
def test_usage(self, *mocks: Any) -> None: | ||
with self.assertRaises(SystemExit): | ||
main() | ||
|
||
out, _ = self.capfd.readouterr() | ||
self.assertTrue(out.startswith("usage:")) | ||
|
||
@patch("argparse._sys.argv", ["run_validation.py", "1.3.6", "--platform", "linux"]) | ||
@patch("src.validation_workflow.validation_tar.ValidationTar.download_artifacts", return_value=True) | ||
@patch("run_validation.main", return_value=0) | ||
def test_main_default(self, mock_tar: Mock, *mocks: Any) -> None: | ||
self.assertEqual(ValidationArgs().version, "1.3.6") | ||
self.assertEqual(ValidationArgs().distribution, "tar") | ||
self.assertNotEqual(ValidationArgs().distribution, "rpm") | ||
|
||
@patch("argparse._sys.argv", ["run_validation.py", "2.1.0", "--distribution", "rpm", "--platform", "linux"]) | ||
@patch("src.validation_workflow.validation_rpm.ValidationRpm.download_artifacts", return_value=True) | ||
@patch("run_validation.main", return_value=0) | ||
def test_main_rpm(self, mock_tar: Mock, *mocks: Any) -> None: | ||
self.assertEqual(ValidationArgs().version, "2.1.0") | ||
self.assertEqual(ValidationArgs().distribution, "rpm") | ||
self.assertNotEqual(ValidationArgs().distribution, "tar") | ||
|
||
@patch("argparse._sys.argv", ["run_validation.py", "2.1.0", "--distribution", "yum", "--platform", "linux"]) | ||
@patch("src.validation_workflow.validation_yum.ValidationYum.download_artifacts", return_value=True) | ||
@patch("run_validation.main", return_value=0) | ||
def test_main_yum(self, mock_tar: Mock, *mocks: Any) -> None: | ||
self.assertEqual(ValidationArgs().version, "2.1.0") | ||
self.assertEqual(ValidationArgs().distribution, "yum") | ||
self.assertNotEqual(ValidationArgs().distribution, "tar") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# Copyright OpenSearch Contributors | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
|
||
import os | ||
import sys | ||
|
||
sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../src")) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
# Copyright OpenSearch Contributors | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
|
||
import logging | ||
import unittest | ||
from unittest.mock import patch | ||
|
||
from validation_workflow.validation_args import ValidationArgs | ||
|
||
|
||
class TestValidationArgs(unittest.TestCase): | ||
|
||
VALIDATION_PY = "./src/run_validation.py" \ | ||
|
||
|
||
@patch("argparse._sys.argv", [VALIDATION_PY, "2.3.0"]) | ||
def test_version(self) -> None: | ||
self.assertTrue(ValidationArgs().version) | ||
self.assertEqual(ValidationArgs().version, "2.3.0") | ||
self.assertNotEqual(ValidationArgs().version, "2.1.0") | ||
|
||
@patch("argparse._sys.argv", [VALIDATION_PY, "2.1.0", "--distribution", "rpm"]) | ||
def test_distribution(self) -> None: | ||
self.assertEqual(ValidationArgs().distribution, "rpm") | ||
|
||
@patch("argparse._sys.argv", [VALIDATION_PY, "1.3.6", "--platform", "linux"]) | ||
def test_platform_default(self) -> None: | ||
self.assertEqual(ValidationArgs().platform, "linux") | ||
|
||
@patch("argparse._sys.argv", [VALIDATION_PY, "1.3.0"]) | ||
def test_verbose_default(self) -> None: | ||
self.assertEqual(ValidationArgs().logging_level, logging.INFO) | ||
|
||
@patch("argparse._sys.argv", [VALIDATION_PY, "1.3.0", "--verbose"]) | ||
def test_verbose_true(self) -> None: | ||
self.assertTrue(ValidationArgs().logging_level, logging.DEBUG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a benefit using 1/0 instead of True/False?
This is also opposite compares to normal programming pattern where 1 is false and 0 is true normally.
Plus, the
else
block can be completely removed in this case.