diff --git a/.github/actions/generate-chart-locks/action.yaml b/.github/actions/generate-chart-locks/action.yaml index dc15d63627..7c1245aee6 100644 --- a/.github/actions/generate-chart-locks/action.yaml +++ b/.github/actions/generate-chart-locks/action.yaml @@ -54,12 +54,12 @@ runs: echo "ref=${resolvedRef}" | tee -a $GITHUB_OUTPUT - name: Checkout id: clone-repository - uses: actions/checkout@v3 + uses: actions/checkout@v4 with: ref: ${{ steps.resolve.outputs.ref }} path: temp-gen-chart-lock-repo - name: Setting up python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: "3.10" - name: Generate lock file JSON from existing charts diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index eea038fbc4..2da4fc8d0a 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -266,7 +266,7 @@ jobs: - name: Get profile version set in report provided by the user id: get-profile-version if: ${{ needs.setup.outputs.run_build == 'true' && steps.verify_requires.outputs.report_provided == 'true' }} - uses: mikefarah/yq@v4 + uses: mikefarah/yq@master with: cmd: yq '.metadata.tool.profile.version' ${{ format('./pr-branch/{0}', steps.verify_requires.outputs.provided_report_relative_path) }} @@ -274,7 +274,7 @@ jobs: id: get-kube-range if: ${{ needs.setup.outputs.run_build == 'true' && steps.verify_requires.outputs.report_provided == 'true' }} continue-on-error: true - uses: mikefarah/yq@v4 + uses: mikefarah/yq@master with: cmd: yq '.metadata.chart.kubeversion' ${{ format('./pr-branch/{0}', steps.verify_requires.outputs.provided_report_relative_path) }} @@ -481,7 +481,7 @@ jobs: # The release tag format is -- - name: Create GitHub release if: ${{ needs.chart-verifier.outputs.web_catalog_only == 'False' }} - uses: softprops/action-gh-release@v0.1.15 + uses: softprops/action-gh-release@v1 with: tag_name: ${{ needs.chart-verifier.outputs.release_tag }} files: | @@ -533,6 +533,23 @@ jobs: https://x-access-token:${{ secrets.GITHUB_TOKEN }}@github.com/${{ github.repository }} \ HEAD:refs/heads/${INDEX_BRANCH} + - name: Add a GitHub comment if release has failed + uses: actions/github-script@v7 + if: ${{ failure() && env.GITHUB_REPOSITORY != 'openshift-helm-charts/sandbox' }} + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + body: `### Release job failed + + An error occured while updating the Helm repository index. + + cc @komish @mgoerens` + }); + # Note(komish): This step is a temporary workaround. Metrics requires the PR comment # to be available, but it is written to the filesystem in the previous job. # This can be removed once the metrics execution is restructured to have access to the PR diff --git a/.github/workflows/check-locks-on-owners-submission.yml b/.github/workflows/check-locks-on-owners-submission.yml index 0e9807a015..682eff97e5 100644 --- a/.github/workflows/check-locks-on-owners-submission.yml +++ b/.github/workflows/check-locks-on-owners-submission.yml @@ -27,7 +27,7 @@ jobs: github.event.pull_request.draft == false steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Set up Python 3.x Part 1 uses: actions/setup-python@v5 @@ -168,7 +168,7 @@ jobs: | - | - | | ${{ steps.gather-metadata.outputs.chart-name }} | ${{ steps.determine-lock-status.outputs.locked-to-path }} | - This OWNERS file is being ${{ steps.populate-file-mod-type.outputs.net-new-owners-file && '**created**' || '**modified**'}} in this pull request. + This OWNERS file is being ${{ steps.populate-file-mod-type.outputs.net-new-owners-file == 'true' && '**created**' || '**modified**'}} in this pull request. _This comment was auto-generated by [GitHub Actions](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})._ diff --git a/.github/workflows/lock-sanity-check.yml b/.github/workflows/lock-sanity-check.yml index f3696d33ae..923417aaa7 100644 --- a/.github/workflows/lock-sanity-check.yml +++ b/.github/workflows/lock-sanity-check.yml @@ -36,7 +36,7 @@ jobs: - name: notify maintainers on failure id: notify if: failure() && steps.generate.outcome == 'failure' && github.repository == 'openshift-helm-charts/charts' - uses: archive/github-actions-slack@v2.7.0 + uses: archive/github-actions-slack@v2.8.0 with: slack-bot-user-oauth-access-token: ${{ secrets.SLACK_BOT_USER_OAUTH_ACCESS_TOKEN }} slack-channel: C02979BDUPL diff --git a/.github/workflows/mercury_bot.yml b/.github/workflows/mercury_bot.yml index da09c27db3..c712bfdd28 100644 --- a/.github/workflows/mercury_bot.yml +++ b/.github/workflows/mercury_bot.yml @@ -79,7 +79,7 @@ jobs: - name: Determine if net-new OWNERS file id: populate-file-mod-type if: ${{ steps.check_for_owners.outputs.merge_pr == 'true' }} - uses: actions/github-script@v6 + uses: actions/github-script@v7 with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | @@ -105,7 +105,7 @@ jobs: # Only used to assert content of the OWNERS file. - name: Checkout Pull Request - uses: actions/checkout@v3 + uses: actions/checkout@v4 with: ref: ${{ github.event.pull_request.head.ref }} repository: ${{ github.event.pull_request.head.repo.full_name }} diff --git a/.github/workflows/owners-redhat.yml b/.github/workflows/owners-redhat.yml index 29540fab7f..b888ff1bae 100644 --- a/.github/workflows/owners-redhat.yml +++ b/.github/workflows/owners-redhat.yml @@ -5,7 +5,7 @@ name: Red Hat OWNERS Files # submissions. on: - pull_request: + pull_request_target: paths: - charts/redhat/**/OWNERS - charts/community/redhat/**/OWNERS @@ -23,10 +23,10 @@ jobs: runs-on: ubuntu-22.04 steps: - name: Clean up stale label - uses: actions/github-script@v3 + uses: actions/github-script@v7 continue-on-error: true with: - github-token: ${{secrets.GITHUB_TOKEN}} + github-token: ${{ secrets.GITHUB_TOKEN }} script: | const successLabel = '${{ env.SUCCESS_LABEL }}'; try { @@ -46,10 +46,10 @@ jobs: throw error } - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Set up Python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: "3.10" @@ -81,23 +81,35 @@ jobs: needs: [gather-metadata] steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Set up Python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: "3.10" - name: Install Python CI tooling working-directory: scripts run: | make venv.tools + + # Only used to assert content of the OWNERS file. + - name: Checkout Pull Request + uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.ref }} + repository: ${{ github.event.pull_request.head.repo.full_name }} + token: ${{ secrets.BOT_TOKEN }} + fetch-depth: 0 + path: "pr-branch" + - name: Assert Owners File Content id: assert-content + working-directory: pr-branch env: CATEGORY: ${{ needs.gather-metadata.outputs.category }} ORGANIZATION: ${{ needs.gather-metadata.outputs.organization }} CHARTNAME: ${{ needs.gather-metadata.outputs.chartname }} run: | - ./scripts/venv.tools/bin/assert-redhat-owners-file-meta \ + ../scripts/venv.tools/bin/assert-redhat-owners-file-meta \ "${CATEGORY}" \ "${ORGANIZATION}" \ "${CHARTNAME}" @@ -108,7 +120,7 @@ jobs: if: failure() && (needs.ensure-prefix-in-path.result == 'failure' || needs.ensure-owners-file-contents.result == 'failure') steps: - name: Send PR Comment - uses: actions/github-script@v6 + uses: actions/github-script@v7 env: PREFIX_CHECK_RESULT: ${{ needs.ensure-prefix-in-path.result != '' && needs.ensure-prefix-in-path.result || 'unknown' }} CONTENT_CHECK_RESULT: ${{ needs.ensure-owners-file-contents.result != '' && needs.ensure-owners-file-contents.result || 'unknown' }} @@ -149,7 +161,7 @@ jobs: if: success() steps: - name: Label PR - uses: actions/github-script@v6 + uses: actions/github-script@v7 with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | diff --git a/.github/workflows/owners.yml b/.github/workflows/owners.yml index e8680070e3..f57b360276 100644 --- a/.github/workflows/owners.yml +++ b/.github/workflows/owners.yml @@ -17,7 +17,7 @@ jobs: uses: actions/checkout@v4 - name: Set up Python 3.x Part 1 - uses: actions/setup-python@4 + uses: actions/setup-python@v5 with: python-version: "3.10" diff --git a/scripts/requirements.txt b/scripts/requirements.txt index 98e38b4ffd..ef110623c0 100644 --- a/scripts/requirements.txt +++ b/scripts/requirements.txt @@ -25,6 +25,7 @@ pytest-forked==1.3.0 pytest-xdist==2.4.0 PyYAML==6.0.1 requests==2.26.0 +responses==0.23.1 retrying==1.3.3 semantic-version==2.8.5 semver==2.13.0 diff --git a/scripts/src/checkprcontent/checkpr.py b/scripts/src/checkprcontent/checkpr.py index 6973880e22..29932af7d8 100644 --- a/scripts/src/checkprcontent/checkpr.py +++ b/scripts/src/checkprcontent/checkpr.py @@ -113,7 +113,7 @@ def get_file_match_compiled_patterns(): pattern = re.compile(base + r"/.*") reportpattern = re.compile(base + r"/report.yaml") - tarballpattern = re.compile(base + r"/(.*\.tgz$)") + tarballpattern = re.compile(base + r"/(.*\.tgz)") return pattern, reportpattern, tarballpattern diff --git a/scripts/src/metrics/pushowners.py b/scripts/src/metrics/pushowners.py index d1d8b2937b..8f49dd8f6a 100644 --- a/scripts/src/metrics/pushowners.py +++ b/scripts/src/metrics/pushowners.py @@ -7,6 +7,10 @@ from owners import owners_file +def bool_to_yes_no(my_bool): + return "Yes" if my_bool else "No" + + def getVendorType(changed_file): path_as_list = changed_file.split("/") for i in range(len(path_as_list) - 1): @@ -16,28 +20,25 @@ def getVendorType(changed_file): def getFileContent(changed_file): - status, owner_data = owners_file.get_owner_data_from_file(changed_file) - if status is True: - users_included = owners_file.get_users_included(owner_data) - web_catalog_only = owners_file.get_web_catalog_only(owner_data) - if not web_catalog_only: - web_catalog_only_string = "No" - else: - web_catalog_only_string = "Yes" - vendor_name = owners_file.get_vendor(owner_data) - chart_name = owners_file.get_chart(owner_data) - vendor_type = getVendorType(changed_file) - return ( - users_included, - web_catalog_only_string, - vendor_name, - chart_name, - vendor_type, - ) - else: - print("Exception loading OWNERS file") + try: + owner_data = owners_file.get_owner_data_from_file(changed_file) + except owners_file.OwnersFileError as e: + print("Exception loading OWNERS file: {e}") return "", "", "", "", "" + users_included = owners_file.get_users_included(owner_data) + web_catalog_only = owners_file.get_web_catalog_only(owner_data) + vendor_name = owners_file.get_vendor(owner_data) + chart_name = owners_file.get_chart(owner_data) + vendor_type = getVendorType(changed_file) + return ( + bool_to_yes_no(users_included), + bool_to_yes_no(web_catalog_only), + vendor_name, + chart_name, + vendor_type, + ) + def process_pr(added_file, modified_file): if modified_file != "": diff --git a/scripts/src/owners/owners_file.py b/scripts/src/owners/owners_file.py index f8913cd1e4..c348922a11 100644 --- a/scripts/src/owners/owners_file.py +++ b/scripts/src/owners/owners_file.py @@ -1,3 +1,4 @@ +import contextlib import os import yaml @@ -8,70 +9,92 @@ from yaml import SafeLoader +class OwnersFileError(Exception): + pass + + +class ConfigKeyMissing(Exception): + pass + + def get_owner_data(category, organization, chart): path = os.path.join("charts", category, organization, chart, "OWNERS") - status, owner_content = get_owner_data_from_file(path) - return status, owner_content + success = False + + try: + owner_content = get_owner_data_from_file(path) + success = True + except OwnersFileError as e: + print(f"Error getting OWNERS file data: {e}") + + return success, owner_content def get_owner_data_from_file(owner_path): try: with open(owner_path) as owner_data: owner_content = yaml.load(owner_data, Loader=SafeLoader) - return True, owner_content - except Exception as err: - print(f"Exception loading OWNERS file: {err}") - return False, "" + except yaml.YAMLError as e: + print(f"Exception loading OWNERS file: {e}") + raise OwnersFileError from e + except OSError as e: + print(f"Error opening OWNERS file: {e}") + raise OwnersFileError from e + + return owner_content def get_vendor(owner_data): - vendor = "" - try: - vendor = owner_data["vendor"]["name"] - except Exception: - pass - return vendor + vendor_name = "" + with contextlib.suppress(KeyError): + vendor_name = owner_data["vendor"]["name"] + return vendor_name def get_vendor_label(owner_data): - vendor = "" - try: - vendor = owner_data["vendor"]["label"] - except Exception: - pass - return vendor + vendor_label = "" + with contextlib.suppress(KeyError): + vendor_label = owner_data["vendor"]["label"] + return vendor_label def get_chart(owner_data): chart = "" - try: + with contextlib.suppress(KeyError): chart = owner_data["chart"]["name"] - except Exception: - pass return chart -def get_web_catalog_only(owner_data): - web_catalog_only = False - try: - if "webCatalogOnly" in owner_data: - web_catalog_only = owner_data["webCatalogOnly"] - elif "providerDelivery" in owner_data: - web_catalog_only = owner_data["providerDelivery"] - except Exception: - pass - return web_catalog_only +def get_web_catalog_only(owner_data, raise_if_missing=False): + """Check the delivery method set in the OWNERS file data + + Args: + owner_data (dict): Content of the OWNERS file. Typically this is the return value of the + get_owner_data or get_owner_data_from_file function. + raise_if_missing (bool, optional): Whether to raise an Exception if the delivery method is + not set in the OWNERS data. If set to False, the function returns False. + + Raises: + ConfigKeyMissing: if the key is not found in OWNERS and raise_if_missing is set to True + + """ + if ( + "web_catalog_only" not in owner_data + and "providerDelivery" not in owner_data + and raise_if_missing + ): + raise ConfigKeyMissing( + "Neither web_catalog_only nor providerDelivery keys were set" + ) + + return owner_data.get("web_catalog_only", False) or owner_data.get( + "providerDelivery", False + ) def get_users_included(owner_data): - users_included = "No" - try: - users = owner_data["users"] - if len(users) != 0: - return "Yes" - except Exception: - pass - return users_included + users = owner_data.get("users", list()) + return len(users) != 0 def get_pgp_public_key(owner_data): diff --git a/scripts/src/precheck/__init__.py b/scripts/src/precheck/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/scripts/src/precheck/serializer.py b/scripts/src/precheck/serializer.py new file mode 100644 index 0000000000..8dba1b6a25 --- /dev/null +++ b/scripts/src/precheck/serializer.py @@ -0,0 +1,47 @@ +"""Contains the logic to serialize / deserialize a Submission object to / from JSON. + +A pair of custom JSONEncoder / JSONDecoder is required due to the fact that the Submission class +contains nested classes. + +""" + +import copy +import json + +from precheck import submission + + +class SubmissionEncoder(json.JSONEncoder): + def default(self, obj): + if isinstance(obj, submission.Submission): + obj_dict = copy.deepcopy(obj.__dict__) + obj_dict["chart"] = obj_dict["chart"].__dict__ + obj_dict["report"] = obj_dict["report"].__dict__ + obj_dict["source"] = obj_dict["source"].__dict__ + obj_dict["tarball"] = obj_dict["tarball"].__dict__ + return obj_dict + + return json.JSONEncoder.default(self, obj) + + +class SubmissionDecoder(json.JSONDecoder): + def __init__(self, *args, **kwargs): + json.JSONDecoder.__init__(self, object_hook=self.object_hook, *args, **kwargs) + + def object_hook(self, dct): + if "chart" in dct: + chart_obj = submission.Chart(**dct["chart"]) + report_obj = submission.Report(**dct["report"]) + source_obj = submission.Source(**dct["source"]) + tarball_obj = submission.Tarball(**dct["tarball"]) + + to_merge_dct = { + "chart": chart_obj, + "report": report_obj, + "source": source_obj, + "tarball": tarball_obj, + } + + new_dct = dct | to_merge_dct + return submission.Submission(**new_dct) + return dct diff --git a/scripts/src/precheck/serializer_test.py b/scripts/src/precheck/serializer_test.py new file mode 100644 index 0000000000..51cf4ed708 --- /dev/null +++ b/scripts/src/precheck/serializer_test.py @@ -0,0 +1,98 @@ +import json + +from precheck import serializer +from precheck import submission + +submission_json = """ +{ + "api_url": "https://api.github.com/repos/openshift-helm-charts/charts/pulls/1", + "modified_files": ["charts/partners/acme/awesome/1.42.0/report.yaml"], + "chart": { + "category": "partners", + "organization": "acme", + "name": "awesome", + "version": "1.42.0" + }, + "report": { + "found": true, + "signed": false, + "path": "charts/partners/acme/awesome/1.42.0/report.yaml" + }, + "source": { + "found": false, + "path": null + }, + "tarball": { + "found": false, + "path": null, + "provenance": null + }, + "modified_owners": [], + "modified_unknown": [], + "is_web_catalog_only": true +} +""" + + +def sanitize_json_string(json_string: str): + """Remove the newlines from the JSON string. This is done by + loading and dumping the string representation of the JSON object. + Goal is to allow comparison with other JSON string. + """ + json_dict = json.loads(json_string) + return json.dumps(json_dict) + + +def test_submission_serializer(): + s = json.loads(submission_json, cls=serializer.SubmissionDecoder) + + assert isinstance(s, submission.Submission) + assert ( + s.api_url == "https://api.github.com/repos/openshift-helm-charts/charts/pulls/1" + ) + assert "charts/partners/acme/awesome/1.42.0/report.yaml" in s.modified_files + assert s.chart.category == "partners" + assert s.chart.organization == "acme" + assert s.chart.name == "awesome" + assert s.chart.version == "1.42.0" + assert s.report.found + assert not s.report.signed + assert s.report.path == "charts/partners/acme/awesome/1.42.0/report.yaml" + assert not s.source.found + assert not s.source.path + assert not s.tarball.found + assert not s.tarball.path + assert not s.tarball.provenance + assert s.is_web_catalog_only + + +def test_submission_deserializer(): + s = submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/1", + modified_files=["charts/partners/acme/awesome/1.42.0/report.yaml"], + chart=submission.Chart( + category="partners", + organization="acme", + name="awesome", + version="1.42.0", + ), + report=submission.Report( + found=True, + signed=False, + path="charts/partners/acme/awesome/1.42.0/report.yaml", + ), + source=submission.Source( + found=False, + path=None, + ), + tarball=submission.Tarball( + found=False, + path=None, + provenance=None, + ), + is_web_catalog_only=True, + ) + + assert serializer.SubmissionEncoder().encode(s) == sanitize_json_string( + submission_json + ) diff --git a/scripts/src/precheck/submission.py b/scripts/src/precheck/submission.py new file mode 100644 index 0000000000..2ed6175aaf --- /dev/null +++ b/scripts/src/precheck/submission.py @@ -0,0 +1,441 @@ +import os +import re +import semver + +from dataclasses import dataclass, field + +from checkprcontent import checkpr +from owners import owners_file +from tools import gitutils +from reporegex import matchers +from report import verifier_report + +xRateLimit = "X-RateLimit-Limit" +xRateRemain = "X-RateLimit-Remaining" + + +class SubmissionError(Exception): + """Root Exception for handling any error with the submission""" + + pass + + +class DuplicateChartError(SubmissionError): + """This Exception is to be raised when the user attempts to submit a PR with more than one chart""" + + pass + + +class VersionError(SubmissionError): + """This Exception is to be raised when the version of the chart is not semver compatible""" + + pass + + +class WebCatalogOnlyError(SubmissionError): + pass + + +@dataclass +class Chart: + """Represents a Helm Chart + + Once set, the category, organization, name and version of the chart cannot be modified. + + """ + + category: str = None + organization: str = None + name: str = None + version: str = None + + def register_chart_info(self, category, organization, name, version): + if ( + (self.category and self.category != category) + or (self.organization and self.organization != organization) + or (self.name and self.name != name) + or (self.version and self.version != version) + ): + msg = "[ERROR] A PR must contain only one chart. Current PR includes files for multiple charts." + raise DuplicateChartError(msg) + + if not semver.VersionInfo.isvalid(version): + msg = ( + f"[ERROR] Helm chart version is not a valid semantic version: {version}" + ) + raise VersionError(msg) + + self.category = category + self.organization = organization + self.name = name + self.version = version + + def get_owners_path(self): + return f"charts/{self.category}/{self.organization}/{self.name}/OWNERS" + + +@dataclass +class Report: + found: bool = False + signed: bool = False + path: str = None + + +@dataclass +class Source: + found: bool = False + path: str = None # Path to the Chart.yaml + + +@dataclass +class Tarball: + found: bool = False + path: str = None + provenance: str = None + + +@dataclass +class Submission: + """Represents a GitHub PR, opened to either certify a new Helm chart or add / modify an OWNERS file. + + A Submission can be instantiated either: + * by solely providing the URL of a given PR (represented by the api_url attribute). Upon + initialization (see __post_init__ method), the rest of the information is retrieved from the + GitHub API. This should typically occur once per pipeline run, at the start. + * by providing all class attributes. This is typically done by loading a JSON representation of + a Submission from a file, and should be done several times per pipeline runs, in later jobs. + + """ + + api_url: str + modified_files: list[str] = None + chart: Chart = field(default_factory=lambda: Chart()) + report: Report = field(default_factory=lambda: Report()) + source: Source = field(default_factory=lambda: Source()) + tarball: Tarball = field(default_factory=lambda: Tarball()) + modified_owners: list[str] = field(default_factory=list) + modified_unknown: list[str] = field(default_factory=list) + is_web_catalog_only: bool = None + + def __post_init__(self): + """Complete the initialization of the Submission object. + + Only retrieve PR information from the GitHub API if requiered, by checking for the presence + of a value for the modified_files attributes. This check allows to make the distinction + between the two aforementioned cases of initialization of a Submission object: + * If modified_files is not set, we're in the case of initializing a brand new Submission + and need to retrieve the rest of the information from the GitHub API. + * If a value is set for modified_files, that means we are loading an existing Submission + object from a file. + + """ + if not self.modified_files: + self.modified_files = [] + self._get_modified_files() + self._parse_modified_files() + + def _get_modified_files(self): + """Query the GitHub API in order to retrieve the list of files that are added / modified by + this PR""" + page_number = 1 + max_page_size, page_size = 100, 100 + files_api_url = re.sub(r"^https://api\.github\.com/", "", self.api_url) + + while page_size == max_page_size: + files_api_query = ( + f"{files_api_url}/files?per_page={page_size}&page={page_number}" + ) + print(f"[INFO] Query files : {files_api_query}") + + try: + r = gitutils.github_api( + "get", files_api_query, os.environ.get("BOT_TOKEN") + ) + except SystemExit as e: + raise SubmissionError(e) + + files = r.json() + page_size = len(files) + page_number += 1 + + if xRateLimit in r.headers: + print(f"[DEBUG] {xRateLimit} : {r.headers[xRateLimit]}") + if xRateRemain in r.headers: + print(f"[DEBUG] {xRateRemain} : {r.headers[xRateRemain]}") + + if "message" in files: + msg = f'[ERROR] getting pr files: {files["message"]}' + raise SubmissionError(msg) + else: + for file in files: + if "filename" in file: + self.modified_files.append(file["filename"]) + + def _parse_modified_files(self): + """Classify the list of modified files. + + Modified files are categorized into 5 groups, mapping to 5 class attributes: + - The `report` attribute has information about files related to the chart-verifier report: + the report.yaml itself and, if signed, its signature report.yaml.asc. + - The `source` attribute has information about files related to the chart's source: all + files, if any, under the src/ directory. + - The `tarball` attribute has information about files related to the chart's source as + tarball: the .tgz tarball itself and, if signed, the .prov provenance file. + - A list of added / modified OWNERS files is recorded in the `modified_owners` attribute. + - The rest of the files are classified in the `modified_unknown` attribute. + + Raises a SubmissionError if: + * The Submission concerns more than one chart + * The version of the chart is not SemVer compatible + * The tarball file is named incorrectly + + """ + for file_path in self.modified_files: + file_category, match = get_file_type(file_path) + if file_category == "report": + self.chart.register_chart_info(*match.groups()) + self.set_report(file_path) + elif file_category == "source": + self.chart.register_chart_info(*match.groups()) + self.set_source(file_path) + elif file_category == "tarball": + category, organization, name, version, _ = match.groups() + self.chart.register_chart_info(category, organization, name, version) + self.set_tarball(file_path, match) + elif file_category == "owners": + self.modified_owners.append(file_path) + elif file_category == "unknwown": + self.modified_unknown.append(file_path) + + def set_report(self, file_path): + """Action to take when a file related to the chart-verifier is found. + + This can either be the report.yaml itself, or the signing key report.yaml.asc + + """ + if os.path.basename(file_path) == "report.yaml": + print(f"[INFO] Report found: {file_path}") + self.report.found = True + self.report.path = file_path + elif os.path.basename(file_path) == "report.yaml.asc": + self.report.signed = True + else: + self.modified_unknown.append(file_path) + + def set_source(self, file_path): + """Action to take when a file related to the chart's source is found. + + Note that while the source of the Chart can be composed of many files, only the Chart.yaml + is actually registered. + + """ + if os.path.basename(file_path) == "Chart.yaml": + self.source.found = True + self.source.path = file_path + + def set_tarball(self, file_path, tarball_match): + """Action to take when a file related to the tarball is found. + + This can either be the .tgz tarball itself, or the .prov provenance key. + + """ + _, file_extension = os.path.splitext(file_path) + if file_extension == ".tgz": + print(f"[INFO] tarball found: {file_path}") + self.tarball.found = True + self.tarball.path = file_path + + _, _, chart_name, chart_version, tar_name = tarball_match.groups() + expected_tar_name = f"{chart_name}-{chart_version}.tgz" + if tar_name != expected_tar_name: + msg = f"[ERROR] the tgz file is named incorrectly. Expected: {expected_tar_name}. Got: {tar_name}" + raise SubmissionError(msg) + elif file_extension == ".prov": + self.tarball.provenance = file_path + else: + self.modified_unknown.append(file_path) + + def is_valid_certification_submission(self): + """Check wether the files in this Submission are valid to attempt to certify a Chart + + We expect the user to provide either: + * Only a report file + * Only a chart - either as source or tarball + * Both the report and the chart + + Returns False if: + * The user attempts to create the OWNERS file for its project. + * The PR contains additional files, not related to the Chart being submitted + + Returns True in all other cases + + """ + if self.modified_owners: + return False, "[ERROR] Send OWNERS file by itself in a separate PR." + + if self.modified_unknown: + msg = ( + "[ERROR] PR includes one or more files not related to charts: " + + ", ".join(self.modified_unknown) + ) + return False, msg + + if self.report.found or self.source.found or self.tarball.found: + return True, "" + + return False, "" + + def is_valid_owners_submission(self): + """Check wether the file in this Submission are valid for an OWNERS PR + + Returns True if the PR only modified files is an OWNERS file. + + Returns False in all other cases. + """ + if len(self.modified_owners) == 1 and len(self.modified_files) == 1: + return True, "" + + msg = "" + if self.modified_owners: + msg = "[ERROR] Send OWNERS file by itself in a separate PR." + else: + msg = "No OWNERS file provided" + + return False, msg + + def parse_web_catalog_only(self, repo_path=""): + """Set the web_catalog_only attribute + + This is achieved by: + - Parsing the associated OWNERS file and check the value of the WebCatalogOnly key + - Parsing report file (if submitted) and check the value of the WebCatalogOnly key + + Args: + repo_path (str): path under which the repo has been cloned on the local filesystem + + Returns: + bool: True if WebCatalog is set to True in both the OWNERS and in the report file. + False if WebCatalogOnly is set to False in the OWNERS file + + Raise: + WebCatalogOnlyError in one of the following cases: + * The OWNERS file doesn't exist at the expected path + * The OWNERS file doesn't contain WebCatalogOnly + * The submitted report cannot be found or read at the expected path (although + report.found is set to True) + * The submitted report doesn't contain WebCatalogOnly + * The WebCatalogOnly key don't match between the OWNERS and the report files + + """ + owners_path = os.path.join(repo_path, self.chart.get_owners_path()) + + try: + owners_data = owners_file.get_owner_data_from_file(owners_path) + except owners_file.OwnersFileError as e: + raise WebCatalogOnlyError( + f"Failed to get OWNERS data at {owners_path}" + ) from e + + try: + owners_web_catalog_only = owners_file.get_web_catalog_only( + owners_data, raise_if_missing=True + ) + except owners_file.ConfigKeyMissing as e: + raise WebCatalogOnlyError( + f"Failed to find webCatalogOnly key in OWNERS data at {owners_path}" + ) from e + + if self.report.found: + report_path = os.path.join(repo_path, self.report.path) + + found, report_data = verifier_report.get_report_data(report_path) + if not found: + raise WebCatalogOnlyError(f"Failed to get report data at {report_path}") + + try: + report_web_catalog_only = verifier_report.get_web_catalog_only( + report_data, raise_if_missing=True + ) + except verifier_report.ConfigKeyMissing as e: + raise WebCatalogOnlyError( + f"Failed to find webCatalogOnly key in report data at {owners_path}" + ) from e + print( + f"[INFO] webCatalogOnly/providerDelivery from report : {report_web_catalog_only}" + ) + + if not owners_web_catalog_only == report_web_catalog_only: + raise WebCatalogOnlyError( + f"Value of web_catalog_only in OWNERS ({owners_web_catalog_only}) doesn't match the value in report ({report_web_catalog_only})" + ) + + self.is_web_catalog_only = owners_web_catalog_only + + def is_valid_web_catalog_only(self, repo_path=""): + """Verify that the submission is coherent with being a WebCatalogOnly submission + + A valid web_catalog_only submission must: + * contain only a report + * the report must specify a package digest + + Args: + repo_path (str): path under which the repo has been cloned on the local filesystem + + Returns: + bool: set to True if the submission is a valid WebCatalogOnly submission. + + Raise: + WebCatalogOnlyError if the submitted report cannot be found or read at the expected path. + + """ + if not self.report.found: + return False + + if len(self.modified_files) > 1: + return False + + report_path = os.path.join(repo_path, self.report.path) + found, report_data = verifier_report.get_report_data(report_path) + if not found: + raise WebCatalogOnlyError(f"Failed to get report data at {report_path}") + + return verifier_report.get_package_digest(report_data) is not None + + +def get_file_type(file_path): + """Determine the category of a given file + + As part of a PR, a modified file can relate to one of 5 categories: + - The chart-verifier report + - The source of the chart + - The tarball of the chart + - OWNERS file + - or another "unknown" category + + """ + pattern, reportpattern, tarballpattern = checkpr.get_file_match_compiled_patterns() + owners_pattern = re.compile( + matchers.submission_path_matcher(include_version_matcher=False) + r"/OWNERS" + ) + src_pattern = re.compile(matchers.submission_path_matcher() + r"/src/") + + # Match all files under charts//// + match = pattern.match(file_path) + if match: + report_match = reportpattern.match(file_path) + if report_match: + return "report", report_match + + src_match = src_pattern.match(file_path) + if src_match: + return "source", src_match + + tar_match = tarballpattern.match(file_path) + if tar_match: + return "tarball", tar_match + else: + owners_match = owners_pattern.match(file_path) + if owners_match: + return "owners", owners_match + + return "unknwown", None diff --git a/scripts/src/precheck/submission_test.py b/scripts/src/precheck/submission_test.py new file mode 100644 index 0000000000..161b048063 --- /dev/null +++ b/scripts/src/precheck/submission_test.py @@ -0,0 +1,620 @@ +"""Unit tests for the Submission Class + +Each test is run against a list of "Scenarios": +- A dataclass named after the function under test and suffixed with "Scenario" is defined +- A list of scenarios to be run against the function under test is created +- The test function is called using the pytest.mark.parametrize decorator + +""" + +import contextlib +import pytest +import os +import responses +import tempfile + +from dataclasses import dataclass, field + +from precheck import submission + +# Define assets that are being reused accross tests +expected_category = "partners" +expected_organization = "acme" +expected_name = "awesome" +expected_version = "1.42.0" + +expected_chart = submission.Chart( + category=expected_category, + organization=expected_organization, + name=expected_name, + version=expected_version, +) + + +def make_new_report_only_submission(): + """Return an initialized Submission that contains only an unsigned report file + + This is a relatively common use case that is used for many scenarios. + """ + s = submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/1", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/report.yaml" + ], + chart=expected_chart, + report=submission.Report( + found=True, + signed=False, + path=f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/report.yaml", + ), + ) + + return s + + +def make_new_tarball_only_submission(): + """Return an initialized Submission that contains only an unsigned tarball""" + s = submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/4", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/{expected_name}-{expected_version}.tgz" + ], + chart=expected_chart, + tarball=submission.Tarball( + found=True, + provenance=None, + path=f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/{expected_name}-{expected_version}.tgz", + ), + ) + return s + + +@dataclass +class SubmissionInitScenario: + api_url: str + modified_files: list[str] + expected_submission: submission.Submission = None + excepted_exception: contextlib.ContextDecorator = field( + default_factory=lambda: contextlib.nullcontext() + ) + + +scenarios_submission_init = [ + # PR contains a unique and unsigned report.yaml + SubmissionInitScenario( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/1", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/report.yaml" + ], + expected_submission=make_new_report_only_submission(), + ), + # PR contains a signed report + SubmissionInitScenario( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/2", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/report.yaml", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/report.yaml.asc", + ], + expected_submission=submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/2", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/report.yaml", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/report.yaml.asc", + ], + chart=expected_chart, + report=submission.Report( + found=True, + signed=True, + path=f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/report.yaml", + ), + ), + ), + # PR contains the chart's source + SubmissionInitScenario( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/3", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/Chart.yaml", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/templates/buildconfig.yam" + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/templates/deployment.yaml", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/templates/imagestream.yam" + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/templates/route.yaml", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/templates/service.yaml", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/values.schema.json", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/values.yaml", + ], + expected_submission=submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/3", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/Chart.yaml", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/templates/buildconfig.yam" + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/templates/deployment.yaml", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/templates/imagestream.yam" + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/templates/route.yaml", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/templates/service.yaml", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/values.schema.json", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/values.yaml", + ], + chart=expected_chart, + source=submission.Source( + found=True, + path=f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/src/Chart.yaml", + ), + ), + ), + # PR contains an unsigned tarball + SubmissionInitScenario( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/4", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/{expected_name}-{expected_version}.tgz" + ], + expected_submission=make_new_tarball_only_submission(), + ), + # PR contains a signed tarball + SubmissionInitScenario( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/5", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/{expected_name}-{expected_version}.tgz", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/{expected_name}-{expected_version}.tgz.prov", + ], + expected_submission=submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/5", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/{expected_name}-{expected_version}.tgz", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/{expected_name}-{expected_version}.tgz.prov", + ], + chart=expected_chart, + tarball=submission.Tarball( + found=True, + provenance=f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/{expected_name}-{expected_version}.tgz.prov", + path=f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/{expected_name}-{expected_version}.tgz", + ), + ), + ), + # PR contains an OWNERS file + SubmissionInitScenario( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/6", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/OWNERS" + ], + expected_submission=submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/6", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/OWNERS" + ], + modified_owners=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/OWNERS" + ], + ), + ), + # PR contains additional files, not fitting into any expected category + SubmissionInitScenario( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/7", + modified_files=["charts/path/to/some/file"], + expected_submission=submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/7", + modified_files=["charts/path/to/some/file"], + modified_unknown=["charts/path/to/some/file"], + ), + ), + # Invalid PR contains multiple reports, referencing multiple charts + SubmissionInitScenario( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/101", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/report.yaml", + f"charts/{expected_category}/{expected_organization}/other-chart/{expected_version}/report.yaml", + ], + excepted_exception=pytest.raises(submission.DuplicateChartError), + ), + # Invalid PR contains a tarball with an incorrect name + SubmissionInitScenario( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/102", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/incorrectly-named.tgz" + ], + excepted_exception=pytest.raises(submission.SubmissionError), + ), + # Invalid PR references a Chart with a version that is not Semver compatible + SubmissionInitScenario( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/103", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/0.1.2.3.4/report.yaml" + ], + excepted_exception=pytest.raises(submission.VersionError), + ), +] + + +@pytest.mark.parametrize("test_scenario", scenarios_submission_init) +@responses.activate +def test_submission_init(test_scenario): + """Test the instantiation of a Submission in different scenarios""" + + # Mock GitHub API + responses.get( + f"{test_scenario.api_url}/files", + json=[{"filename": file} for file in test_scenario.modified_files], + ) + + with test_scenario.excepted_exception: + s = submission.Submission(api_url=test_scenario.api_url) + assert s == test_scenario.expected_submission + + +@responses.activate +def test_submission_not_exist(): + """Test creating a Submission for an unexisting PR""" + + api_url_doesnt_exist = ( + "https://api.github.com/repos/openshift-helm-charts/charts/pulls/9999" + ) + + responses.get( + f"{api_url_doesnt_exist}/files", + json={ + "message": "Not Found", + "documentation_url": "https://docs.github.com/rest/pulls/pulls#list-pull-requests-files", + }, + ) + + with pytest.raises(submission.SubmissionError): + submission.Submission(api_url=api_url_doesnt_exist) + + +@dataclass +class CertificationScenario: + input_submission: submission.Submission + expected_is_valid_certification: bool + expected_reason: str = "" + + +scenarios_certification_submission = [ + # Valid certification Submission contains only a report + CertificationScenario( + input_submission=make_new_report_only_submission(), + expected_is_valid_certification=True, + ), + # Invalid certification Submission contains OWNERS file + CertificationScenario( + input_submission=submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/1", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/OWNERS" + ], + modified_owners=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/OWNERS" + ], + ), + expected_is_valid_certification=False, + expected_reason="[ERROR] Send OWNERS file by itself in a separate PR.", + ), + # Invalid certification Submission contains unknown files + CertificationScenario( + input_submission=submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/1", + modified_files=["charts/path/to/some/file"], + modified_unknown=["charts/path/to/some/file"], + ), + expected_is_valid_certification=False, + expected_reason="[ERROR] PR includes one or more files not related to charts:", + ), +] + + +@pytest.mark.parametrize("test_scenario", scenarios_certification_submission) +def test_is_valid_certification(test_scenario): + is_valid_certification, reason = ( + test_scenario.input_submission.is_valid_certification_submission() + ) + assert test_scenario.expected_is_valid_certification == is_valid_certification + assert test_scenario.expected_reason in reason + + +@dataclass +class OwnersScenario: + input_submission: submission.Submission + expected_is_valid_owners: bool + expected_reason: str = "" + + +scenarios_owners_submission = [ + # Valid submission contains only one OWNERS file + OwnersScenario( + input_submission=submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/1", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/OWNERS" + ], + modified_owners=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/OWNERS" + ], + ), + expected_is_valid_owners=True, + ), + # Invalid submission contains multiple OWNERS file + OwnersScenario( + input_submission=submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/1", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/OWNERS", + f"charts/{expected_category}/{expected_organization}/another_chart/OWNERS", + ], + modified_owners=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/OWNERS", + f"charts/{expected_category}/{expected_organization}/another_chart/OWNERS", + ], + ), + expected_is_valid_owners=False, + expected_reason="[ERROR] Send OWNERS file by itself in a separate PR.", + ), + # Invalid submission contains unknown files + OwnersScenario( + input_submission=make_new_report_only_submission(), + expected_is_valid_owners=False, + expected_reason="No OWNERS file provided", + ), + # Invalid submission doesn't contain an OWNER file +] + + +@pytest.mark.parametrize("test_scenario", scenarios_owners_submission) +def test_is_valid_owners(test_scenario): + is_valid_owners, reason = ( + test_scenario.input_submission.is_valid_owners_submission() + ) + assert test_scenario.expected_is_valid_owners == is_valid_owners + assert test_scenario.expected_reason in reason + + +@dataclass +class WebCatalogOnlyScenario: + input_submission: submission.Submission + create_owners: bool = True + create_report: bool = True + owners_web_catalog_only: str = None # Set to None to skip key creation in OWNERS + report_web_catalog_only: str = None # Set to None to skip key creation in report + expected_output: bool = None + excepted_exception: contextlib.ContextDecorator = field( + default_factory=lambda: contextlib.nullcontext() + ) + + +scenarios_web_catalog_only = [ + # Submission contains a report file with WebCatalogOnly set to True, matching the content of OWNERS + WebCatalogOnlyScenario( + input_submission=make_new_report_only_submission(), + owners_web_catalog_only="true", + report_web_catalog_only="true", + expected_output=True, + ), + # Submission contains a report file with WebCatalogOnly set to False, matching the content of OWNERS + WebCatalogOnlyScenario( + input_submission=make_new_report_only_submission(), + owners_web_catalog_only="false", + report_web_catalog_only="false", + expected_output=False, + ), + # Submission contains a report file with WebCatalogOnly set to True, not matching the content of OWNERS + WebCatalogOnlyScenario( + input_submission=make_new_report_only_submission(), + owners_web_catalog_only="true", + report_web_catalog_only="false", + excepted_exception=pytest.raises( + submission.WebCatalogOnlyError, match="doesn't match the value" + ), + ), + # Submission contains a report file with WebCatalogOnly set to False, not matching the content of OWNERS + WebCatalogOnlyScenario( + input_submission=make_new_report_only_submission(), + owners_web_catalog_only="false", + report_web_catalog_only="true", + excepted_exception=pytest.raises( + submission.WebCatalogOnlyError, match="doesn't match the value" + ), + ), + # Submission doesn't relate to an existing OWNERS + WebCatalogOnlyScenario( + input_submission=make_new_report_only_submission(), + create_owners=False, + report_web_catalog_only="true", + excepted_exception=pytest.raises( + submission.WebCatalogOnlyError, match="Failed to get OWNERS data" + ), + ), + # Submission contains a report, but it can't be found + WebCatalogOnlyScenario( + input_submission=make_new_report_only_submission(), + owners_web_catalog_only="true", + create_report=False, + excepted_exception=pytest.raises( + submission.WebCatalogOnlyError, match="Failed to get report data" + ), + ), + # The OWNERS file for this chart doesn't contain the WebCatalogOnly key + WebCatalogOnlyScenario( + input_submission=make_new_report_only_submission(), + owners_web_catalog_only=None, + report_web_catalog_only="false", + excepted_exception=pytest.raises(submission.WebCatalogOnlyError), + ), + # Submission contains a report, that doesn't contain the WebCatalogOnly key + WebCatalogOnlyScenario( + input_submission=make_new_report_only_submission(), + owners_web_catalog_only=None, + report_web_catalog_only="false", + excepted_exception=pytest.raises(submission.WebCatalogOnlyError), + ), + # Submission doesn't contain a report, OWNERS file has WebCatalogOnly set to True + # Note that this is not a valid scenario: if webCatalogOnly is set to True in the OWNERS file, + # all chart submissions should contain only a report. This check is part of the + # is_valid_web_catalog_only method. + WebCatalogOnlyScenario( + input_submission=make_new_tarball_only_submission(), + owners_web_catalog_only="true", + report_web_catalog_only=None, + expected_output=True, + ), + # Submission doesn't contain a report, OWNERS file has WebCatalogOnly set to False + WebCatalogOnlyScenario( + input_submission=make_new_tarball_only_submission(), + owners_web_catalog_only="false", + report_web_catalog_only=None, + expected_output=False, + ), +] + + +@pytest.mark.parametrize("test_scenario", scenarios_web_catalog_only) +def test_parse_web_catalog_only(test_scenario): + """Use a temporary directory, which content mimic the certification repository + + A OWNERS file and a report.yaml are placed at the correct location, containing the minimum + information required for this test to function (a webCatalogOnly value). + + """ + with tempfile.TemporaryDirectory(dir=".") as temp_dir: + # Create directory structure + owners_base_path = os.path.join( + temp_dir, + "charts", + expected_category, + expected_organization, + expected_name, + ) + chart_base_path = os.path.join( + owners_base_path, + expected_version, + ) + os.makedirs(chart_base_path) + + # Populate OWNERS file + if test_scenario.create_owners: + owners_file = open(os.path.join(owners_base_path, "OWNERS"), "w") + owners_file.write( + "publicPgpKey: unknown" + ) # Make sure OWNERS is not an empty file + if test_scenario.owners_web_catalog_only: + owners_file.write( + f"\nproviderDelivery: {test_scenario.owners_web_catalog_only}" + ) + owners_file.close() + + # Populate report.yaml file + if test_scenario.create_report: + report_file = open(os.path.join(chart_base_path, "report.yaml"), "w") + report_file.writelines( + [ + "apiversion: v1", + "\nkind: verify-report", + ] + ) + if test_scenario.report_web_catalog_only: + report_file.writelines( + [ + "\nmetadata:", + "\n tool:", + f"\n webCatalogOnly: {test_scenario.report_web_catalog_only}", + ] + ) + report_file.close() + + with test_scenario.excepted_exception: + test_scenario.input_submission.parse_web_catalog_only(repo_path=temp_dir) + + assert ( + test_scenario.input_submission.is_web_catalog_only + == test_scenario.expected_output + ) + + +@dataclass +class IsWebCatalogOnlyScenario: + input_submission: submission.Submission + create_report: bool = True + report_has_digest: bool = None + expected_output: bool = None + + +scenarios_is_web_catalog_only = [ + # Submission contains only a report, and report contains a digest + IsWebCatalogOnlyScenario( + input_submission=make_new_report_only_submission(), + report_has_digest=True, + expected_output=True, + ), + # Submission contains only a report, but report contains no digest + IsWebCatalogOnlyScenario( + input_submission=make_new_report_only_submission(), + report_has_digest=False, + expected_output=False, + ), + # Submission contains no report + IsWebCatalogOnlyScenario( + input_submission=make_new_tarball_only_submission(), + create_report=False, + expected_output=False, + ), + # Submission contains a report and other files + IsWebCatalogOnlyScenario( + input_submission=submission.Submission( + api_url="https://api.github.com/repos/openshift-helm-charts/charts/pulls/1", + modified_files=[ + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/report.yaml", + f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/{expected_name}-{expected_version}.tgz", + ], + chart=expected_chart, + report=submission.Report( + found=True, + signed=False, + path=f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/report.yaml", + ), + tarball=submission.Tarball( + found=True, + provenance=None, + path=f"charts/{expected_category}/{expected_organization}/{expected_name}/{expected_version}/{expected_name}-{expected_version}.tgz", + ), + ), + report_has_digest=True, + expected_output=False, + ), +] + + +@pytest.mark.parametrize("test_scenario", scenarios_is_web_catalog_only) +def test_is_valid_web_catalog_only(test_scenario): + + with tempfile.TemporaryDirectory(dir=".") as temp_dir: + # Create directory structure + chart_base_path = os.path.join( + temp_dir, + "charts", + expected_category, + expected_organization, + expected_name, + expected_version, + ) + os.makedirs(chart_base_path) + + # Populate report.yaml file + if test_scenario.create_report: + report_file = open(os.path.join(chart_base_path, "report.yaml"), "w") + report_file.writelines( + [ + "apiversion: v1", + "\nkind: verify-report", + ] + ) + if test_scenario.report_has_digest: + report_file.writelines( + [ + "\nmetadata:", + "\n tool:", + "\n digests:", + "\n package: 7755e7cf43e55bbf2edafd9788b773b844fb15626c5ff8ff7a30a6d9034f3a33", + ] + ) + report_file.close() + + assert ( + test_scenario.input_submission.is_valid_web_catalog_only(repo_path=temp_dir) + == test_scenario.expected_output + ) diff --git a/scripts/src/report/verifier_report.py b/scripts/src/report/verifier_report.py index 875444e6a9..cd3a7a863b 100644 --- a/scripts/src/report/verifier_report.py +++ b/scripts/src/report/verifier_report.py @@ -43,6 +43,10 @@ KUBE_VERSION_ATTRIBUTE = "kubeVersion" +class ConfigKeyMissing(Exception): + pass + + def get_report_data(report_path): """Load and returns the report data contained in report.yaml @@ -105,20 +109,41 @@ def get_profile_version(report_data): return profile_version -def get_web_catalog_only(report_data): +def get_web_catalog_only(report_data, raise_if_missing=False): + """Check the delivery method set in the report data. + + Args: + report_data (dict): Content of the report file. Typically this is the return value of the + get_report_data function. + raise_if_missing (bool, optional): Whether to raise an Exception if the delivery method is + not set in the report data. If set to False, the function returns False. + + Raises: + ConfigKeyMissing: if the key is not found in OWNERS and raise_if_missing is set to True + + """ + keyFound = False web_catalog_only = False try: if "webCatalogOnly" in report_data["metadata"]["tool"]: web_catalog_only = report_data["metadata"]["tool"]["webCatalogOnly"] + keyFound = True if "providerControlledDelivery" in report_data["metadata"]["tool"]: web_catalog_only = report_data["metadata"]["tool"][ "providerControlledDelivery" ] + keyFound = True except Exception as err: print( f"Exception getting webCatalogOnly/providerControlledDelivery {err=}, {type(err)=}" ) pass + + if not keyFound and raise_if_missing: + raise ConfigKeyMissing( + "Neither webCatalogOnly nor providerControlledDelivery keys were set" + ) + return web_catalog_only diff --git a/scripts/src/signedchart/signedchart.py b/scripts/src/signedchart/signedchart.py index fc852f880a..7df533562a 100644 --- a/scripts/src/signedchart/signedchart.py +++ b/scripts/src/signedchart/signedchart.py @@ -65,19 +65,14 @@ def is_chart_signed(api_url, report_path): return False -def key_in_owners_match_report(owner_path, report_path): - owner_key = get_pgp_key_from_owners(owner_path) - if not owner_key: - return True - return check_pgp_public_key(owner_key, report_path) - - def get_pgp_key_from_owners(owner_path): - found, owner_data = owners_file.get_owner_data_from_file(owner_path) - if found: - pgp_key = owners_file.get_pgp_public_key(owner_data) - return pgp_key - return "" + try: + owner_data = owners_file.get_owner_data_from_file(owner_path) + except owners_file.OwnersFileError: + return "" + + pgp_key = owners_file.get_pgp_public_key(owner_data) + return pgp_key def check_report_for_signed_chart(report_path): diff --git a/tests/data/HC-16/dash-in-version/partner/report.yaml b/tests/data/HC-16/dash-in-version/partner/report.yaml deleted file mode 100644 index b4aec81305..0000000000 --- a/tests/data/HC-16/dash-in-version/partner/report.yaml +++ /dev/null @@ -1,89 +0,0 @@ -apiversion: v1 -kind: verify-report -metadata: - tool: - verifier-version: 1.7.0 - profile: - VendorType: partner - version: v1.1 - chart-uri: https://github.com/openshift-helm-charts/development/blob/main/tests/data/psql-service-0.1.10-1.tgz?raw=true - digests: - chart: sha256:db482b4d90349c6b276ba27f581720cc62fa7bea05184b5bfd840844178a8da6 - package: c8635dcdc8f8493abbdef85305be55c9d5dbf3a44495a88a8285c9a0d0c63408 - lastCertifiedTimestamp: "2022-06-22T15:54:59.964823+00:00" - testedOpenShiftVersion: "4.10" - supportedOpenShiftVersions: '>=4.7' - providerControlledDelivery: false - chart: - name: psql-service - home: "" - sources: [] - version: 0.1.10-1 - description: A Helm chart for a RedHat Certified PSQL - keywords: [] - maintainers: [] - icon: "" - apiversion: v2 - condition: "" - tags: "" - appversion: 10.0.0 - deprecated: false - annotations: - charts.openshift.io/archs: x86_64 - charts.openshift.io/name: PSQL RedHat Demo Chart - charts.openshift.io/provider: RedHat - charts.openshift.io/supportURL: https://github.com/dperaza4dustbit/helm-chart - kubeversion: '>=1.20.0' - dependencies: [] - type: application - chart-overrides: "" -results: - - check: v1.0/contains-test - type: Mandatory - outcome: PASS - reason: Chart test files exist - - check: v1.0/contains-values - type: Mandatory - outcome: PASS - reason: Values file exist - - check: v1.1/has-kubeversion - type: Mandatory - outcome: PASS - reason: Kubernetes version specified - - check: v1.0/not-contains-crds - type: Mandatory - outcome: PASS - reason: Chart does not contain CRDs - - check: v1.0/not-contain-csi-objects - type: Mandatory - outcome: PASS - reason: CSI objects do not exist - - check: v1.0/chart-testing - type: Mandatory - outcome: PASS - reason: Chart tests have passed - - check: v1.0/has-readme - type: Mandatory - outcome: PASS - reason: Chart has a README - - check: v1.0/is-helm-v3 - type: Mandatory - outcome: PASS - reason: API version is V2, used in Helm 3 - - check: v1.0/contains-values-schema - type: Mandatory - outcome: PASS - reason: Values schema file exist - - check: v1.0/helm-lint - type: Mandatory - outcome: PASS - reason: Helm lint successful - - check: v1.0/images-are-certified - type: Mandatory - outcome: PASS - reason: 'Image is Red Hat certified : registry.access.redhat.com/rhscl/postgresql-10-rhel7:1-66' - - check: v1.0/required-annotations-present - type: Mandatory - outcome: PASS - reason: All required annotations present - diff --git a/tests/data/HC-16/dash-in-version/redhat/report.yaml b/tests/data/HC-16/dash-in-version/redhat/report.yaml deleted file mode 100644 index eff7b99361..0000000000 --- a/tests/data/HC-16/dash-in-version/redhat/report.yaml +++ /dev/null @@ -1,89 +0,0 @@ -apiversion: v1 -kind: verify-report -metadata: - tool: - verifier-version: 1.7.0 - profile: - VendorType: redhat - version: v1.1 - chart-uri: https://github.com/openshift-helm-charts/development/blob/main/tests/data/psql-service-0.1.10-1.tgz?raw=true - digests: - chart: sha256:db482b4d90349c6b276ba27f581720cc62fa7bea05184b5bfd840844178a8da6 - package: c8635dcdc8f8493abbdef85305be55c9d5dbf3a44495a88a8285c9a0d0c63408 - lastCertifiedTimestamp: "2022-06-22T17:21:03.1478+00:00" - testedOpenShiftVersion: "4.10" - supportedOpenShiftVersions: '>=4.7' - providerControlledDelivery: false - chart: - name: psql-service - home: "" - sources: [] - version: 0.1.10-1 - description: A Helm chart for a RedHat Certified PSQL - keywords: [] - maintainers: [] - icon: "" - apiversion: v2 - condition: "" - tags: "" - appversion: 10.0.0 - deprecated: false - annotations: - charts.openshift.io/archs: x86_64 - charts.openshift.io/name: PSQL RedHat Demo Chart - charts.openshift.io/provider: RedHat - charts.openshift.io/supportURL: https://github.com/dperaza4dustbit/helm-chart - kubeversion: '>=1.20.0' - dependencies: [] - type: application - chart-overrides: "" -results: - - check: v1.0/has-readme - type: Mandatory - outcome: PASS - reason: Chart has a README - - check: v1.0/is-helm-v3 - type: Mandatory - outcome: PASS - reason: API version is V2, used in Helm 3 - - check: v1.0/not-contains-crds - type: Mandatory - outcome: PASS - reason: Chart does not contain CRDs - - check: v1.0/not-contain-csi-objects - type: Mandatory - outcome: PASS - reason: CSI objects do not exist - - check: v1.0/images-are-certified - type: Mandatory - outcome: PASS - reason: 'Image is Red Hat certified : registry.access.redhat.com/rhscl/postgresql-10-rhel7:1-66' - - check: v1.0/chart-testing - type: Mandatory - outcome: PASS - reason: Chart tests have passed - - check: v1.0/required-annotations-present - type: Mandatory - outcome: PASS - reason: All required annotations present - - check: v1.0/contains-test - type: Mandatory - outcome: PASS - reason: Chart test files exist - - check: v1.0/contains-values - type: Mandatory - outcome: PASS - reason: Values file exist - - check: v1.0/contains-values-schema - type: Mandatory - outcome: PASS - reason: Values schema file exist - - check: v1.1/has-kubeversion - type: Mandatory - outcome: PASS - reason: Kubernetes version specified - - check: v1.0/helm-lint - type: Mandatory - outcome: PASS - reason: Helm lint successful - diff --git a/tests/data/HC-17/plus-in-version/partner/report.yaml b/tests/data/HC-17/plus-in-version/partner/report.yaml new file mode 100644 index 0000000000..44761b1b8c --- /dev/null +++ b/tests/data/HC-17/plus-in-version/partner/report.yaml @@ -0,0 +1,120 @@ +apiversion: v1 +kind: verify-report +metadata: + tool: + verifier-version: 1.13.4 + profile: + VendorType: partner + version: v1.3 + reportDigest: uint64:4054779787745617517 + chart-uri: https://github.com/openshift-helm-charts/development/blob/main/tests/data/cryostat-0.4.0%2B1.tgz?raw=true + digests: + chart: sha256:6b5babbd4329410e21cf6ae9011bf517c38352592aceea06c227988656e0bdb0 + package: e210e4887fd7ed5cdfece4844f73f827ef69afa09d9cda8963232022f194efa2 + lastCertifiedTimestamp: "2024-05-22T15:18:04.379024-04:00" + testedOpenShiftVersion: "4.14" + supportedOpenShiftVersions: '>=4.6' + webCatalogOnly: false + chart: + name: cryostat + home: https://cryostat.io + sources: + - https://github.com/cryostatio/cryostat + - https://github.com/cryostatio/cryostat-core + - https://github.com/cryostatio/cryostat-web + - https://github.com/cryostatio/jfr-datasource + - https://github.com/cryostatio/cryostat-grafana-dashboard + version: 0.4.0+1 + description: Securely manage JFR recordings for your containerized Java workloads + keywords: + - flightrecorder + - java + - jdk + - jfr + - jmc + - missioncontrol + - monitoring + - profiling + - diagnostic + maintainers: + - name: The Cryostat Community + email: "" + url: https://groups.google.com/g/cryostat-development + icon: https://raw.githubusercontent.com/cryostatio/cryostat-helm/main/docs/images/cryostat-icon.svg + apiversion: v2 + condition: "" + tags: "" + appversion: 2.4.0.redhat + deprecated: false + annotations: + charts.openshift.io/archs: x86_64 + charts.openshift.io/digest: sha256:e5f987974557cb190c02579e7dfd0c56c8715dc5e45ddcfd2af944f57c38c150 + charts.openshift.io/lastCertifiedTimestamp: "2024-02-21T18:36:30.02064+00:00" + charts.openshift.io/name: Red Hat build of Cryostat + charts.openshift.io/provider: RedHat + charts.openshift.io/providerType: redhat + charts.openshift.io/supportURL: https://github.com/cryostatio/cryostat-helm + charts.openshift.io/supportedOpenShiftVersions: '>=4.6' + charts.openshift.io/testedOpenShiftVersion: "4.14" + kubeversion: '>= 1.19.0-0' + dependencies: [] + type: application + chart-overrides: "" +results: + - check: v1.0/contains-test + type: Mandatory + outcome: PASS + reason: Chart test files exist + - check: v1.0/is-helm-v3 + type: Mandatory + outcome: PASS + reason: API version is V2, used in Helm 3 + - check: v1.0/required-annotations-present + type: Mandatory + outcome: PASS + reason: All required annotations present + - check: v1.0/signature-is-valid + type: Mandatory + outcome: SKIPPED + reason: 'Chart is not signed : Signature verification not required' + - check: v1.0/contains-values-schema + type: Mandatory + outcome: PASS + reason: Values schema file exist + - check: v1.0/has-readme + type: Mandatory + outcome: PASS + reason: Chart has a README + - check: v1.1/images-are-certified + type: Mandatory + outcome: PASS + reason: No images to certify + - check: v1.0/not-contain-csi-objects + type: Mandatory + outcome: PASS + reason: CSI objects do not exist + - check: v1.1/has-kubeversion + type: Mandatory + outcome: PASS + reason: Kubernetes version specified + - check: v1.0/chart-testing + type: Mandatory + outcome: PASS + reason: Chart tests have passed + - check: v1.0/contains-values + type: Mandatory + outcome: PASS + reason: Values file exist + - check: v1.0/helm-lint + type: Mandatory + outcome: PASS + reason: Helm lint successful + - check: v1.0/not-contains-crds + type: Mandatory + outcome: PASS + reason: Chart does not contain CRDs + - check: v1.0/has-notes + type: Optional + outcome: PASS + reason: Chart does contain NOTES.txt + diff --git a/tests/data/HC-17/plus-in-version/redhat/report.yaml b/tests/data/HC-17/plus-in-version/redhat/report.yaml new file mode 100644 index 0000000000..c17577386d --- /dev/null +++ b/tests/data/HC-17/plus-in-version/redhat/report.yaml @@ -0,0 +1,120 @@ +apiversion: v1 +kind: verify-report +metadata: + tool: + verifier-version: 1.13.4 + profile: + VendorType: redhat + version: v1.3 + reportDigest: uint64:5964035041144068952 + chart-uri: https://github.com/openshift-helm-charts/development/blob/main/tests/data/cryostat-0.4.0%2B1.tgz?raw=true + digests: + chart: sha256:6b5babbd4329410e21cf6ae9011bf517c38352592aceea06c227988656e0bdb0 + package: e210e4887fd7ed5cdfece4844f73f827ef69afa09d9cda8963232022f194efa2 + lastCertifiedTimestamp: "2024-05-22T15:18:41.525481-04:00" + testedOpenShiftVersion: "4.14" + supportedOpenShiftVersions: '>=4.6' + webCatalogOnly: false + chart: + name: cryostat + home: https://cryostat.io + sources: + - https://github.com/cryostatio/cryostat + - https://github.com/cryostatio/cryostat-core + - https://github.com/cryostatio/cryostat-web + - https://github.com/cryostatio/jfr-datasource + - https://github.com/cryostatio/cryostat-grafana-dashboard + version: 0.4.0+1 + description: Securely manage JFR recordings for your containerized Java workloads + keywords: + - flightrecorder + - java + - jdk + - jfr + - jmc + - missioncontrol + - monitoring + - profiling + - diagnostic + maintainers: + - name: The Cryostat Community + email: "" + url: https://groups.google.com/g/cryostat-development + icon: https://raw.githubusercontent.com/cryostatio/cryostat-helm/main/docs/images/cryostat-icon.svg + apiversion: v2 + condition: "" + tags: "" + appversion: 2.4.0.redhat + deprecated: false + annotations: + charts.openshift.io/archs: x86_64 + charts.openshift.io/digest: sha256:e5f987974557cb190c02579e7dfd0c56c8715dc5e45ddcfd2af944f57c38c150 + charts.openshift.io/lastCertifiedTimestamp: "2024-02-21T18:36:30.02064+00:00" + charts.openshift.io/name: Red Hat build of Cryostat + charts.openshift.io/provider: RedHat + charts.openshift.io/providerType: redhat + charts.openshift.io/supportURL: https://github.com/cryostatio/cryostat-helm + charts.openshift.io/supportedOpenShiftVersions: '>=4.6' + charts.openshift.io/testedOpenShiftVersion: "4.14" + kubeversion: '>= 1.19.0-0' + dependencies: [] + type: application + chart-overrides: "" +results: + - check: v1.1/has-kubeversion + type: Mandatory + outcome: PASS + reason: Kubernetes version specified + - check: v1.0/signature-is-valid + type: Mandatory + outcome: SKIPPED + reason: 'Chart is not signed : Signature verification not required' + - check: v1.0/contains-values-schema + type: Mandatory + outcome: PASS + reason: Values schema file exist + - check: v1.0/contains-test + type: Mandatory + outcome: PASS + reason: Chart test files exist + - check: v1.0/not-contain-csi-objects + type: Mandatory + outcome: PASS + reason: CSI objects do not exist + - check: v1.0/required-annotations-present + type: Mandatory + outcome: PASS + reason: All required annotations present + - check: v1.0/contains-values + type: Mandatory + outcome: PASS + reason: Values file exist + - check: v1.0/not-contains-crds + type: Mandatory + outcome: PASS + reason: Chart does not contain CRDs + - check: v1.1/images-are-certified + type: Mandatory + outcome: PASS + reason: No images to certify + - check: v1.0/helm-lint + type: Mandatory + outcome: PASS + reason: Helm lint successful + - check: v1.0/is-helm-v3 + type: Mandatory + outcome: PASS + reason: API version is V2, used in Helm 3 + - check: v1.0/has-notes + type: Optional + outcome: PASS + reason: Chart does contain NOTES.txt + - check: v1.0/has-readme + type: Mandatory + outcome: PASS + reason: Chart has a README + - check: v1.0/chart-testing + type: Mandatory + outcome: PASS + reason: Chart tests have passed + diff --git a/tests/data/cryostat-0.4.0+1.tgz b/tests/data/cryostat-0.4.0+1.tgz new file mode 100644 index 0000000000..d26c0788ad Binary files /dev/null and b/tests/data/cryostat-0.4.0+1.tgz differ diff --git a/tests/functional/behave_features/HC-17_dash_in_version.feature b/tests/functional/behave_features/HC-17_dash_in_version.feature index 7431fce613..58eb3d689f 100644 --- a/tests/functional/behave_features/HC-17_dash_in_version.feature +++ b/tests/functional/behave_features/HC-17_dash_in_version.feature @@ -13,9 +13,25 @@ Feature: Report only submission Examples: | vendor_type | vendor | report_path | | partners | redhat | tests/data/HC-17/dash-in-version/partner/report.yaml | - + @redhat @full Examples: | vendor_type | vendor | report_path | | redhat | redhat | tests/data/HC-17/dash-in-version/redhat/report.yaml | + Scenario Outline: [HC-17-002] A partner or redhat associate submits report only with plus in chart version + Given the vendor "" has a valid identity as "" + And an error-free report is used in "" + When the user sends a pull request with the report + Then the user sees the pull request is merged + And the index.yaml file is updated with an entry for the submitted chart + + @partners @full + Examples: + | vendor_type | vendor | report_path | + | partners | redhat | tests/data/HC-17/plus-in-version/partner/report.yaml | + + @redhat @full + Examples: + | vendor_type | vendor | report_path | + | redhat | redhat | tests/data/HC-17/plus-in-version/redhat/report.yaml | diff --git a/tests/functional/behave_features/common/utils/chart_certification.py b/tests/functional/behave_features/common/utils/chart_certification.py index 3414a53ab7..15c2491cca 100644 --- a/tests/functional/behave_features/common/utils/chart_certification.py +++ b/tests/functional/behave_features/common/utils/chart_certification.py @@ -135,9 +135,15 @@ def send_pull_request(self, remote_repo, base_branch, pr_branch, bot_token): logging.debug(f"PR_BODY Content: {pr_body}") logging.info(f"Create PR from '{remote_repo}:{pr_branch}'") r = github_api("post", f"repos/{remote_repo}/pulls", bot_token, json=data) - j = json.loads(r.text) + + try: + j = json.loads(r.text) + except json.JSONDecodeError as e: + raise AssertionError(f"error decoding GitHub response: {r.__dict__}") from e + if "number" not in j: raise AssertionError(f"error sending pull request, response was: {r.text}") + return j["number"] def create_and_push_owners_file( @@ -333,7 +339,7 @@ def check_workflow_conclusion( conclusion = get_run_result(self.secrets, run_id) if conclusion == expect_result: logging.info( - f"PR{pr_number} Workflow run was '{expect_result}' which is expected" + f"PR{pr_number if pr_number else self.secrets.pr_number} Workflow run was '{expect_result}' which is expected" ) else: if failure_type == "warning": diff --git a/tests/functional/behave_features/common/utils/github.py b/tests/functional/behave_features/common/utils/github.py index 5b525e5f1a..5c966d3f97 100644 --- a/tests/functional/behave_features/common/utils/github.py +++ b/tests/functional/behave_features/common/utils/github.py @@ -31,7 +31,9 @@ def get_run_id(secrets, workflow_name: str, pr_number: str = None): logging.debug(f'workflow found with id "{run["id"]}"') return run["id"] else: - raise Exception(f"Workflow for the submitted PR (#{pr_number}) did not run.") + raise Exception( + f"Workflow for the submitted PR (#{pr_number if pr_number else secrets.pr_number}) did not run." + ) @retry(stop_max_delay=60_000 * 40, wait_fixed=2000)