feat: add validation step for strain configuration schema and repository URLs #35

magajh · 2024-10-22T18:43:25Z

Summary

This PR introduces a new step, Validate strain configuration, which includes two Python scripts to validate the strain configuration file. These scripts ensure that the configuration adheres to the required schema and that repo URLs are valid. The goal is to catch errors early, allowing the workflow to display any issues immediately, rather than encountering errors much later in the build process.

Execution

The validation step sequentially runs both scripts:

python $STRAIN_VALIDATION_SCRIPTS_PATH/validate_schema.py $CONFIG_FILE
python $STRAIN_VALIDATION_SCRIPTS_PATH/validate_repos.py $CONFIG_FILE

Schema validation is performed first to catch any missing keys or format errors before proceeding with other checks.

How to Test

Go to the ednx-strains repository, which hosts the caller workflow for Picasso: https://github.com/eduNEXT/ednx-strains/actions
Navigate to Actions > Build Open edX strain and enter the required configuration to manually trigger the workflow.
Under Use workflow from, select the branch magagio/build-workflow-syntax-verification.
In Branch to clone the strain from, you can either use the same branch or create a new one with modifications to the Redwood strain. Examples of modifications to test error handling:
- Use an incorrect repository or version for a private package or an extra pip requirement.
- Set an incorrectly formatted version for Tutor.
- Remove a required key in PICASSO_THEMES.
Click Run workflow.
Verify that the workflow fails at the strain validation step and displays the corresponding error message.

Example Workflow Runs

Non-existent branch in a private requirement
Run example
Missing version key in PICASSO_THEMES (e.g., edunext/redwood.master)
Run example

Exclusions

Repository validation in PICASSO_THEMES: This validation is skipped as it fails clearly within 57 seconds if incorrect.
Run example
Validation of repositories in private packages: These also fail in around one minute if incorrect.
Run example

…hecks

mariajgrimaldi · 2024-11-08T16:58:58Z

.github/workflows/scripts/strain_validation/validate_schema.py

+        "TUTOR_VERSION": And(
+            str,
+            Regex(TUTOR_VERSION_REGEX, error="TUTOR_VERSION must be in the format vX.Y.Z (e.g., v5.3.0)")


Actually, TUTOR_VERSION should not include the v-prefix. See this issue for more context: #20

mariajgrimaldi · 2024-11-08T16:59:55Z

.github/workflows/scripts/strain_validation/validate_schema.py

+        Optional(Regex(r"^PICASSO_.+_DPKG$")): {
+            "name": And(str, len),
+            "repo": And(str, len),
+            "version": And(str, len)
+        },
+        Optional("PICASSO_THEMES"): And(
+            list,
+            len,
+            [
+                {
+                    "name": And(str, len),
+                    "repo": And(str, len),
+                    "version": And(str, len)
+                }
+            ]
+        ),
+        Optional("PICASSO_THEMES_NAME"): And(
+            list,
+            len,
+            lambda x: all(isinstance(item, str) and item for item in x),
+            error=EMPTY_LIST_ERROR_MSG.format("PICASSO_THEMES_NAME"),
+        ),
+        Optional("PICASSO_THEME_DIRS"): And(
+            list,
+            len,
+            lambda x: all(isinstance(item, str) and item for item in x),
+            error=EMPTY_LIST_ERROR_MSG.format("PICASSO_THEME_DIRS"),
+        )


I was under the impression this could be done in tutor-contrib-picasso instead when enabling themes or private packages, was that an approach considered during the implementation? Or is this something that can only be done during the building process and not when executing extra-commands?

mariajgrimaldi · 2024-11-08T17:08:14Z

.github/workflows/scripts/strain_validation/validate_repos.py

+"""
+Validate repository URLs and configurations in the strain config file.
+"""


Can you please add here how this script it's supposed to be used? As we've done here: https://github.com/eduNEXT/picasso/blob/main/.github/workflows/scripts/get_tutor_config.py#L1-L15

mariajgrimaldi · 2024-11-08T17:14:57Z

.github/workflows/scripts/strain_validation/validate_repos.py

+    edx_platform_version = data.get('EDX_PLATFORM_VERSION', "")
+    url = f"{edx_platform_repository}/tree/{edx_platform_version}"
+    if not validate_repo_url(url):
+        LOG.error("Failed to validate edx_platform_repository URL: %s", url)


Suggested change

LOG.error("Failed to validate edx_platform_repository URL: %s", url)

LOG.error("Failed to validate EDX_PLATFORM_REPOSITORY URL: %s", url)

How do we know if the EDX_PLATFORM_REPOSITORY exists but not the EDX_PLATFORM_VERSION?

mariajgrimaldi · 2024-11-08T17:41:06Z

.github/workflows/scripts/strain_validation/validate_repos.py

+        data = yaml.safe_load(strain_file)
+    except yaml.YAMLError:
+        LOG.exception("Error loading YAML data.")
+        return 1


Suggested change

return 1

return EXIT_FAILURE

mariajgrimaldi · 2024-11-08T17:41:25Z

.github/workflows/scripts/strain_validation/validate_repos.py

+        return 1
+
+    LOG.info("All repository URLs validated successfully.")
+    return 0


Suggested change

return 0

return EXIT_SUCCESS

mariajgrimaldi · 2024-11-08T17:41:57Z

.github/workflows/scripts/strain_validation/validate_schema.py

+    if "PICASSO_THEMES" in data and not ("PICASSO_THEMES_NAME" in data or "PICASSO_DEFAULT_SITE_THEME" in data):
+        raise SchemaError("PICASSO_THEMES_NAME or PICASSO_DEFAULT_SITE_THEME must be defined.")
+    return data


Can't this be checkec using the schema somehow?

mariajgrimaldi · 2024-11-08T17:46:48Z

.github/workflows/scripts/strain_validation/validate_repos.py

+    try:
+        data = yaml.safe_load(strain_file)
+    except yaml.YAMLError:
+        LOG.exception("Error loading YAML data.")


I like how you're using logs for getting more details about failures. I'll see how we can fit this into the other scripts. Thank you!

mariajgrimaldi · 2024-11-08T18:02:17Z

.github/workflows/build.yml

@@ -73,14 +74,21 @@ jobs:

      - name: Install necessary dependencies
        run: |
-          pip install pyyaml
+          pip install pyyaml schema


We'll need to merge #30 so we can use the requirements file instead. I'll put a pin on this so I don't forget.

@mariajgrimaldi agree, I left my approval in #30 so we can close it

mariajgrimaldi · 2024-11-08T18:03:19Z

.github/workflows/scripts/strain_validation/validate_schema.py

+    Args:
+        data (Dict[str, Any]): The YAML data dictionary.
+
+    Returns:
+        Dict[str, Any]: Validated data dictionary.
+
+    Raises:
+        SchemaError: If required conditions are not met.


Can we use this format for all functions? I think this one's missing: https://github.com/eduNEXT/picasso/pull/35/files#diff-1fe9dc0e10d81110fac939c4576da08ead8a873a353cf04a3cff270831958499R42

.github/workflows/scripts/strain_validation/validate_schema.py

BryanttV · 2024-11-08T20:10:14Z

.github/workflows/scripts/strain_validation/validate_schema.py

+    Returns:
+        int: 0 if configuration file is valid; 1 if invalid.
+    """
+    parser = argparse.ArgumentParser(description="Validate YAML file syntax and strain schema.")


Could we validate the arguments in another function similar to how it is done here?

BryanttV · 2024-11-08T20:12:14Z

.github/workflows/scripts/strain_validation/validate_schema.py

+        int: 0 if configuration file is valid; 1 if invalid.
+    """
+    parser = argparse.ArgumentParser(description="Validate YAML file syntax and strain schema.")
+    parser.add_argument("file", type=argparse.FileType("r"), nargs="+", help="YAML file to validate.")


Is the nargs=+ necessary? I understand that only the config.yml file will be validated.

BryanttV · 2024-11-08T20:22:39Z

.github/workflows/scripts/strain_validation/validate_repos.py

+        int: 0 if all URLs are validated successfully, 1 if any validation fails.
+    """
+    parser = argparse.ArgumentParser(description="Validate repository URLs in strain config file.")
+    parser.add_argument("file", type=argparse.FileType("r"), nargs="+")


The same as I comment here

Co-authored-by: Bryann Valderrama <[email protected]>

magajh force-pushed the mjh/repo-and-syntx-analysis branch 4 times, most recently from 20edbcc to 9f3e3ea Compare November 8, 2024 08:49

feat: add strain configuration validation step with schema and repo c…

d74f046

…hecks

magajh force-pushed the mjh/repo-and-syntx-analysis branch from bf9432b to d74f046 Compare November 8, 2024 14:03

magajh changed the title ~~feat: add syntax and url verifications~~ feat: add validation step for strain configuration schema and repository URLs Nov 8, 2024

magajh marked this pull request as ready for review November 8, 2024 14:34

magajh requested a review from a team as a code owner November 8, 2024 14:34

magajh requested review from MaferMazu and mariajgrimaldi November 8, 2024 14:34

mariajgrimaldi requested changes Nov 8, 2024

View reviewed changes

BryanttV reviewed Nov 8, 2024

View reviewed changes

fix: remove unused import in validate schema script

2e068b3

Co-authored-by: Bryann Valderrama <[email protected]>

magajh mentioned this pull request Dec 3, 2024

Problem using TUTOR_VERSION eduNEXT/tutor-contrib-picasso#10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add validation step for strain configuration schema and repository URLs #35

feat: add validation step for strain configuration schema and repository URLs #35

magajh commented Oct 22, 2024 •

edited

Loading

mariajgrimaldi Nov 8, 2024

mariajgrimaldi Nov 8, 2024

mariajgrimaldi Nov 8, 2024

mariajgrimaldi Nov 8, 2024

mariajgrimaldi Nov 8, 2024

mariajgrimaldi Nov 8, 2024

mariajgrimaldi Nov 8, 2024

mariajgrimaldi Nov 8, 2024

mariajgrimaldi Nov 8, 2024

magajh Nov 8, 2024

mariajgrimaldi Nov 8, 2024

BryanttV Nov 8, 2024

BryanttV Nov 8, 2024

BryanttV Nov 8, 2024

	LOG.error("Failed to validate edx_platform_repository URL: %s", url)
	LOG.error("Failed to validate EDX_PLATFORM_REPOSITORY URL: %s", url)

feat: add validation step for strain configuration schema and repository URLs #35

Are you sure you want to change the base?

feat: add validation step for strain configuration schema and repository URLs #35

Conversation

magajh commented Oct 22, 2024 • edited Loading

Summary

Execution

How to Test

Example Workflow Runs

Exclusions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magajh commented Oct 22, 2024 •

edited

Loading