Skip to content

feat: introduce GlobalConfig.from_app_manifest#1662

Merged
artemrys merged 1 commit intochore/gc-updatefrom
gc_from_app_manifest
Mar 27, 2025
Merged

feat: introduce GlobalConfig.from_app_manifest#1662
artemrys merged 1 commit intochore/gc-updatefrom
gc_from_app_manifest

Conversation

@artemrys
Copy link
Member

Issue number: ADDON-78488

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

This feature allows UCC to initialize a GlobalConfig from app.manifest file. This will be later used to create a GlobalConfig if there is no such exists which enables UCC to support .conf-only add-ons in a better way.

User experience

N/A

Checklist

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

Review

  • self-review - I have performed a self-review of this change according to the development guidelines
  • Changes are documented. The documentation is understandable, examples work (more info)
  • PR title and description follows the contributing principles
  • meeting - I have scheduled a meeting or recorded a demo to explain these changes (if there is a video, put a link below and in the ticket)

Tests

See the testing doc.

  • Unit - tests have been added/modified to cover the changes
  • Smoke - tests have been added/modified to cover the changes
  • UI - tests have been added/modified to cover the changes
  • coverage - I have checked the code coverage of my changes (see more)

Demo/meeting:

Reviewers are encouraged to request meetings or demos if any part of the change is unclear

@artemrys artemrys marked this pull request as ready for review March 26, 2025 17:19
@artemrys artemrys requested a review from a team as a code owner March 26, 2025 17:19
app_manifest_correct
)

assert expected_global_config == global_config
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of adding comparison method in source code only for tests, we could have a helper method that does the same job as we do in __eq__() method. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I could do that but I like keeping such logic in the class itself.

If I would be writing a test to compare 2 objects - I assume
this object has eq method implemented.

@hetangmodi-crest
Copy link
Contributor

Should we also update .coveragrc as we are targeting to reach 100% coverage for the entire source code?

@artemrys
Copy link
Member Author

Should we also update .coveragrc as we are targeting to reach 100% coverage for the entire source code?

Good catch, I'll update the coverage in the "chore/gc-update" branch.

@artemrys artemrys merged commit f9866c9 into chore/gc-update Mar 27, 2025
91 of 92 checks passed
@artemrys artemrys deleted the gc_from_app_manifest branch March 27, 2025 12:04
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 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