helpers icon indicating copy to clipboard operation
helpers copied to clipboard

HelpersTask676_Add_script_to_synchronize_GH_branch_protection_and_merge_property

Open sandeepthalapanane opened this issue 7 months ago • 5 comments

Addressing #676

Documentation about YAML params - https://docs.google.com/document/d/1Kpe-DVpBjcy7Do4-NmnxlJkRcwjSMqwT5GFpjSE2qvo/edit?usp=sharing

I have included a way to revert GitHub settings to their defaults if they are present in the repo but not listed in the YAML manifest using the switch_default flag.

If a field is not present in the YAML file, NotSet is used to ensure no accidental overwrites.

sandeepthalapanane avatar May 13 '25 22:05 sandeepthalapanane

@heanhsok, could you please review this code?

sandeepthalapanane avatar May 16 '25 20:05 sandeepthalapanane

@heanhsok I tested the script with a test repository, and it works. I was unable to test the users and teams, but the backup file and switching to the default are working correctly.

sandeepthalapanane avatar May 23 '25 06:05 sandeepthalapanane

Could you help check this? @sandeepthalapanane

user_1042@18f837e9d157:/app$ dev_scripts_helpers/github/dockerized_sync_gh_repo_settings.py --token_env_var GITHUB_PAT --input_file dev_scripts_helpers/github/settings/repo_settings.yaml --dry_run --owner HeaSoLab --repo demo-repository --backup
18:19:37 - INFO  hdbg.py init_logger:1013                               Saving log to file '/app/dev_scripts_helpers/github/dockerized_sync_gh_repo_settings.py.log'
18:19:37 - INFO  hdbg.py init_logger:1018                               > cmd='dev_scripts_helpers/github/dockerized_sync_gh_repo_settings.py --token_env_var GITHUB_PAT --input_file dev_scripts_helpers/github/settings/repo_settings.yaml --dry_run --owner HeaSoLab --repo demo-repository --backup'
18:19:38 - INFO  dockerized_sync_gh_repo_settings.py _main:527          Settings backed up to /app/tmp.settings.HeaSoLab.demo-repository.yaml
Are you sure you want to synchronize repository settings? [y/n] y
18:19:40 - INFO  dockerized_sync_gh_repo_settings.py apply_branch_protection:321 Would apply branch protection rules to main:
  strict: True
  contexts: ['ci/lint', 'ci/fasttests']
  enforce_admins: True
  dismiss_stale_reviews: True
  require_code_owner_reviews: True
  required_approving_review_count: 1
  user_push_restrictions: ['admin1', 'admin2']
  team_push_restrictions: ['maintainers', 'developers']
18:19:40 - INFO  dockerized_sync_gh_repo_settings.py apply_repo_settings:415 Would apply repository settings:
  name: test-repo
  description: Test repository with branch protection and settings
  homepage: https://github.com/test-repo
  private: True
  has_issues: True
  has_projects: True
  has_wiki: False
  allow_squash_merge: True
  allow_merge_commit: False
  allow_rebase_merge: True
  archived: False
Traceback (most recent call last):
  File "/app/dev_scripts_helpers/github/dockerized_sync_gh_repo_settings.py", line 556, in <module>
    _main(_parse())
  File "/app/dev_scripts_helpers/github/dockerized_sync_gh_repo_settings.py", line 551, in _main
    settings.apply_repo_settings(repo, args.dry_run)
  File "/app/dev_scripts_helpers/github/dockerized_sync_gh_repo_settings.py", line 419, in apply_repo_settings
    if enable_security_fixes is not None:
       ^^^^^^^^^^^^^^^^^^^^^
UnboundLocalError: cannot access local variable 'enable_security_fixes' where it is not associated with a value

heanhsok avatar May 27 '25 22:05 heanhsok

Could you help check this? @sandeepthalapanane

user_1042@18f837e9d157:/app$ dev_scripts_helpers/github/dockerized_sync_gh_repo_settings.py --token_env_var GITHUB_PAT --input_file dev_scripts_helpers/github/settings/repo_settings.yaml --dry_run --owner HeaSoLab --repo demo-repository --backup
18:19:37 - INFO  hdbg.py init_logger:1013                               Saving log to file '/app/dev_scripts_helpers/github/dockerized_sync_gh_repo_settings.py.log'
18:19:37 - INFO  hdbg.py init_logger:1018                               > cmd='dev_scripts_helpers/github/dockerized_sync_gh_repo_settings.py --token_env_var GITHUB_PAT --input_file dev_scripts_helpers/github/settings/repo_settings.yaml --dry_run --owner HeaSoLab --repo demo-repository --backup'
18:19:38 - INFO  dockerized_sync_gh_repo_settings.py _main:527          Settings backed up to /app/tmp.settings.HeaSoLab.demo-repository.yaml
Are you sure you want to synchronize repository settings? [y/n] y
18:19:40 - INFO  dockerized_sync_gh_repo_settings.py apply_branch_protection:321 Would apply branch protection rules to main:
  strict: True
  contexts: ['ci/lint', 'ci/fasttests']
  enforce_admins: True
  dismiss_stale_reviews: True
  require_code_owner_reviews: True
  required_approving_review_count: 1
  user_push_restrictions: ['admin1', 'admin2']
  team_push_restrictions: ['maintainers', 'developers']
18:19:40 - INFO  dockerized_sync_gh_repo_settings.py apply_repo_settings:415 Would apply repository settings:
  name: test-repo
  description: Test repository with branch protection and settings
  homepage: https://github.com/test-repo
  private: True
  has_issues: True
  has_projects: True
  has_wiki: False
  allow_squash_merge: True
  allow_merge_commit: False
  allow_rebase_merge: True
  archived: False
Traceback (most recent call last):
  File "/app/dev_scripts_helpers/github/dockerized_sync_gh_repo_settings.py", line 556, in <module>
    _main(_parse())
  File "/app/dev_scripts_helpers/github/dockerized_sync_gh_repo_settings.py", line 551, in _main
    settings.apply_repo_settings(repo, args.dry_run)
  File "/app/dev_scripts_helpers/github/dockerized_sync_gh_repo_settings.py", line 419, in apply_repo_settings
    if enable_security_fixes is not None:
       ^^^^^^^^^^^^^^^^^^^^^
UnboundLocalError: cannot access local variable 'enable_security_fixes' where it is not associated with a value

I fixed this. Initially, I ran the script without a dry-run flag. I made sure that after the fix, I dry-ran the script and verified the implementation.

sandeepthalapanane avatar May 28 '25 05:05 sandeepthalapanane

Sorry guys I've reviewed it while you guys were also working on it and tried my best to do a clean merge.

I've proposed several simplifications of the flow. The architecture and the use modes are not hidden.

Even if this is not production code, let's iterate to clarify the patterns as an exercise on understanding how to build elegant architecture.

Pls @sandeepthalapanane make a proposal with @heanhsok. @Shaunak01 keep an eye on this since this is a useful exercise for everyone.

gpsaggese avatar May 30 '25 16:05 gpsaggese

@heanhsok

I have completely restructured the code following the review by @gpsaggese.

The new architecture flow includes:

  1. Two distinct sub-commands: export and sync.

  2. Export Pipeline: get settings → validate →save.

  3. Sync Pipeline: load → validate → diff → log (dry-run) / backup → apply.

  4. The central logic has been encapsulated in a private class called _RepoAndBranchSettings, which builds repository and branch configurations once and reuses them.

  5. Backups are now created unconditionally in the current working directory, ensuring that restore points are always available.

  6. The switch_default has been removed. It seems to be confusing and likely unnecessary, and we already have a common settings file, which looks enough.

Improvements made:

  1. There is now comprehensive logging for dry-run operations, allowing users to see exactly what would change. Additionally, various logs are available for debug mode to better understand the flow of the code.

  2. I have introduced key validation for YAML files (_validate_settings) to catch any unknown setting parameters.

  3. Helper functions (_build_settings, _log_settings) have been factored out to eliminate duplicate code.@heanhsok I have completely restructured the code after @gpsaggese's review.

  4. I have also added current and target prefixes to variable names for clarity. I made sure to use variable names like repo_and_branch_settings, repo_settings, and branch_settings to avoid confusion. For example, repo_and_branch_settings contains both repository and branch protection setting dictionaries, and repo_settings contains only repository settings.

  5. I have added more detailed comments, doc strings, and examples.

sandeepthalapanane avatar Jun 01 '25 07:06 sandeepthalapanane