Adds the monitor scanner #1
Conversation
ArneTR
left a comment
ArneTR
left a comment
There was a problem hiding this comment.
Code is a bit uneasy to read as it is very full of guard clauses and method extractions but overall looks apt for the job.
I added some questions and annotations. After that ready to merge IMHO
git-cron/monitor_repos.py
Outdated
| tmp_path = path + ".tmp" | ||
| with open(tmp_path, "w", encoding="utf-8") as f: | ||
| json.dump(data, f, indent=2, sort_keys=True) | ||
| os.replace(tmp_path, path) |
There was a problem hiding this comment.
what is the use of the tmp file? if a disk error happens it can happen either in the write or in the replace function ... not? So why the increased complexity?
There was a problem hiding this comment.
Because like this it is an atomic operation. It was generated like this and I quite liked the pattern. Changed
| if branch: | ||
| params["sha"] = branch | ||
| try: | ||
| resp = requests.get(api_url, params=params, timeout=timeout) |
There was a problem hiding this comment.
why are you not using the APIClient here?
There was a problem hiding this comment.
The return codes are different. In API client I want to know what fails, here I just error if GitHub goes down.
| if branch: | ||
| params["ref_name"] = branch | ||
| try: | ||
| resp = requests.get(api_url, params=params, timeout=timeout) |
There was a problem hiding this comment.
why are you not using the APIClient here?
| @@ -0,0 +1,22 @@ | |||
| { | |||
There was a problem hiding this comment.
Should this not be a file that is rather a config.json.example and the actual config.json ignored in .gitignore?
|
|
||
| if isinstance(vars_cfg, dict) and vars_cfg: | ||
| for k, v in vars_cfg.items(): | ||
| if v == "__GIT_HASH__": |
There was a problem hiding this comment.
I would document that you can use this "magic variable" in your usage scenario. It might not be directly clear from the config.json.example
There was a problem hiding this comment.
Good Point. Added to the readme
| ) | ||
| p.add_argument( | ||
| "--state", | ||
| default="repo_state.json", |
No description provided.