Skip to content
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

fix: prevent unnecessary updates to the globalConfig file #1535

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

artemrys
Copy link
Member

@artemrys artemrys commented Jan 20, 2025

Issue number: ADDON-77791

PR Type

What kind of change does this PR introduce?

  • Feature
  • Bug Fix
  • Refactoring (no functional or API changes)
  • Documentation Update
  • Maintenance (dependency updates, CI, etc.)

Summary

Changes

Prevent unnecessary updates to the globalConfig file. To reproduce it - create a fresh add-on using init -> build commands and you shall see multiple log lines with "Updated globalConfig schema to version " which should not be needed as it is a completely fresh add-on which should already use the latest version of the globalConfig file.

The old logic relies on _uccVersion field which is also no longer present (#1519) in the globalConfig file.

The new logic explicitly declares a list of supported schema versions (which is not the most elegant solution) and uses "0.0.0" in case schemaVersion is not known (this is possible in add-ons built with older versions of Add-on Builder).

User experience

No unnecessary updates to the globalConfig file.

Checklist

If an item doesn't apply to your changes, leave it unchecked.

@artemrys artemrys marked this pull request as ready for review January 20, 2025 20:25
@artemrys artemrys requested a review from a team as a code owner January 20, 2025 20:25
@vtsvetkov-splunk
Copy link
Contributor

Suggested pull request title: fix: prevent unnecessary updates to globalConfig schema version

Great work on addressing the issue of unnecessary globalConfig updates! The changes effectively solve the problem of repeated schema updates in fresh add-ons while maintaining compatibility with older versions. The code is well-structured and includes appropriate test coverage.

However, there are a few areas where the implementation could be improved:

  1. In global_config_update.py:

    • The allowed_versions_of_schema_version set could be moved to a module-level constant (e.g., SUPPORTED_SCHEMA_VERSIONS) to improve maintainability and make it more visible
    • Consider using an enum or dataclass for version management instead of a set of strings
    • The warning message when version is not in allowed versions could be more informative by including the actual version that was found
  2. In test_global_config_update.py:

    • Add test cases for invalid schema versions to verify the warning behavior
    • Consider adding parameterized tests for different schema version scenarios
    • Add docstrings to the test functions to better describe what they're testing
  3. Documentation:

    • The changes should be documented in the changelog
    • Consider adding a comment explaining why "0.0.0" is used as the fallback version
  4. Code style:

    • Consider using type hints more consistently throughout the code
    • The _version_tuple function could benefit from additional validation of the input string

Example improvement for the version handling:

from enum import Enum
from typing import Set

class SchemaVersion(str, Enum):
    DEFAULT = "0.0.0"
    LATEST = "0.0.9"
    # ... other versions

SUPPORTED_SCHEMA_VERSIONS: Set[str] = {version.value for version in SchemaVersion}

def handle_global_config_update(global_config: global_config_lib.GlobalConfig) -> None:
    version = global_config.schema_version or SchemaVersion.DEFAULT.value
    logger.info(f"Current globalConfig schema version is {version}")

    if version not in SUPPORTED_SCHEMA_VERSIONS:
        logger.warning(
            f"Schema version '{version}' is not in the allowed versions {SUPPORTED_SCHEMA_VERSIONS}, "
            f"falling back to {SchemaVersion.DEFAULT.value}"
        )
        version = SchemaVersion.DEFAULT.value

The changes look good overall and address the core issue effectively. After implementing the suggested improvements, this PR should be ready for merging.

This comment was added by our PR Review Assistant Bot. Please kindly acknowledge that
while we're doing our best to keep these comments up to very high standards, they may occasionally be
incorrect. Suggestions offered by the Bot are only intended as points for consideration and no statements
by this bot alone can be considered grounds for merging of any pull request. Remember to seek a review
from a human co-worker.

To reply to the review and engage Review Bot in further conversation, start your comment with the words Review bot:

Copy link
Contributor

@kkedziak-splunk kkedziak-splunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I did not notice that _uccversion check

@artemrys artemrys merged commit b541693 into develop Jan 21, 2025
122 checks passed
@artemrys artemrys deleted the fix/updater branch January 21, 2025 16:21
@github-actions github-actions bot locked and limited conversation to collaborators Jan 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants