V3.0 - #1
Open
halicki wants to merge 29 commits into
Open
Conversation
This commit includes several changes: - Added a restore functionality to the backup system. - Implemented log coloring based on log levels. - Introduced a new logs.html template. - Made various modifications to the backup, scheduler, and server modules. These changes aim to improve the user experience and functionality of the system.
halicki
commented
Mar 6, 2025
halicki
left a comment
halicki
left a comment
Author
There was a problem hiding this comment.
Największy problem. Nie ma testów. A nie ma testów, bo pewnie ciężko było by je dodać. A ciężko je dodać, bo wszystko zależy od wszystkiego, bo architektura jest trudna. Możemy się ktoregoś dnia zdzwonić, żeby przegadać to.
Comment on lines
+77
to
+97
| @property | ||
| def logger(self) -> logging.Logger: | ||
| """Returns logger for the class. | ||
|
|
||
| Returns: | ||
| logging.Logger: Logger for the class. | ||
| """ | ||
| return self._logger | ||
|
|
||
| @logger.setter | ||
| def logger(self, logger:logging.Logger) -> None: | ||
| """Sets logger for the class. | ||
|
|
||
| Args: | ||
| logger (logging.Logger): Logger for the class. | ||
| """ | ||
| if logger is None: | ||
| logging.config.fileConfig("log_dev.conf") | ||
| self._logger = logging.getLogger('pybackupper_logger') | ||
| else: | ||
| self._logger = logger |
Author
There was a problem hiding this comment.
To jest nadmiarowe. Nie ma konieczności przypisywania loggera do klasy. Możesz na górze po prostu zainicjalizować. Albo kolejnym fajnym podjeściem jest korzystanie tylko z jednego loggera. Który umieściłbyś w logging'u.
Comment on lines
+99
to
+129
| @property | ||
| def name(self) -> str: | ||
| """Returns name of the backup. | ||
|
|
||
| Returns: | ||
| str: Name of the backup. | ||
| """ | ||
| return self._name | ||
|
|
||
| @name.setter | ||
| def name(self, name:str) -> None: | ||
| """Sets name of the backup. | ||
|
|
||
| Args: | ||
| name (str): Name of the backup. | ||
|
|
||
| Raises: | ||
| ValueError: Name of the backup is not valid. | ||
| PermissionError: Change of `name` property is not allowed for Backup. | ||
| """ | ||
| if name is None or name == "": | ||
| self.logger.error(f"Backup {name} is not valid.") | ||
| raise ValueError(f"Backup {name} is not valid.") | ||
|
|
||
| try: | ||
| if self._name != "": | ||
| self.logger.error("Cannot change name of the backup.") | ||
| raise PermissionError("Cannot change name of the backup.") | ||
| except AttributeError: | ||
| self.logger.debug(f"Setting name of the backup to {name}.") | ||
| self._name = name |
Author
There was a problem hiding this comment.
Nadmiar, rzadko kiedy settery i gettery są przydatne w realnym kodzie.
| return self._dest_path | ||
|
|
||
| @dest_path.setter | ||
| def dest_path(self, dest_path:str) -> None: |
Comment on lines
+186
to
+189
| try: | ||
| return self._completed | ||
| except AttributeError: | ||
| return False |
Author
There was a problem hiding this comment.
Nie wiem po co jest ten blok try except.
Comment on lines
+204
to
+205
| self.logger.debug(f"Setting completed property of the backup to {completed}.") | ||
| self._completed = completed |
Author
There was a problem hiding this comment.
W tym miejscu masz błąd logiczny. W tym miejscu jeszcze nie wiemy że back up się zakończył.
Comment on lines
+74
to
+104
| def __str__(self) -> str: | ||
| """Get the string representation of the object. | ||
|
|
||
| Returns: | ||
| str: String representation of the object. | ||
| """ | ||
| local_size = 0 | ||
| s3_size = 0 | ||
|
|
||
| for backup in self.backups["local"]: | ||
| local_size += backup.get_size() | ||
|
|
||
| s3_size = self.s3_handler.get_bucket_size() if not self.s3_handler is None else 0 | ||
|
|
||
| return f"BackupManager:\n" \ | ||
| f" src_path: {self.src_path}\n" \ | ||
| f" dest_path: {self.dest_path}\n" \ | ||
| f" ignored: {self.ignored}\n" \ | ||
| f" raw_to_keep: {self.raw_to_keep}\n" \ | ||
| f" compressed_to_keep: {self.compressed_to_keep}\n" \ | ||
| f" s3_handler: {True if self.s3_handler else False}\n" \ | ||
| f" telegram_handler: {True if self.telegram_handler else False}\n" \ | ||
| f" backups:\n" \ | ||
| f" local:\n" \ | ||
| f" count: {len(self.backups['local'])}\n" \ | ||
| f" size: {size_to_human_readable(local_size)}\n" \ | ||
| f" s3:\n" \ | ||
| f" count: {len(self.backups['s3'])}\n" \ | ||
| f" size: {size_to_human_readable(s3_size)}\n" \ | ||
| f" last:\n" \ | ||
| f" {self.backups['local'][-1].name if len(self.backups['local']) > 0 else None}" |
Author
There was a problem hiding this comment.
Zamiast budować to na piechotę pomyśl jak wyciągnąć klasę która by enkapsulowała informacje potrzebne w tym str. I użyj np. dataclasses do tego.
| self.pending_backup = False | ||
| return True | ||
|
|
||
| def run_backup(self, callback=None) -> str: |
Comment on lines
+1142
to
+1162
| except OSError: | ||
| self.logger.error(f"Backup info cannot be saved.") | ||
| self.logger.error(f"Printing backup info:\n{pformat(self.__dict__(), sort_dicts=False, compact=True, indent=2)}") | ||
|
|
||
| if self.telegram_handler: | ||
| self.telegram_handler.send_message( | ||
| f"*Backup info cannot be saved\\.*\n" \ | ||
| f"Printing backup info:\n```json\n{pformat(self.__dict__(), sort_dicts=False, compact=True, indent=2)}```\n", | ||
| markdown=True) | ||
| except botocoreClientError: | ||
| self.logger.error(f"Backup info cannot be uploaded to S3.") | ||
| self.logger.error(f"Printing backup info:\n{pformat(self.__dict__(), sort_dicts=False, compact=True, indent=2)}") | ||
|
|
||
| if self.telegram_handler: | ||
| self.telegram_handler.send_message( | ||
| f"*Backup info cannot be uploaded to S3\\.*\n" \ | ||
| f"Printing backup info:\n```json\n{pformat(self.__dict__(), sort_dicts=False, compact=True, indent=2)}```\n", | ||
| markdown=True) | ||
|
|
||
| self.pending_backup = False | ||
| return self.backups["local"][-1].name |
Author
There was a problem hiding this comment.
To powinno być triggerowane po zakonczeniu processowania, nie jako element wykonania backapu.
Author
There was a problem hiding this comment.
https://www.cosmicpython.com/book/chapter_11_external_events.html rzuć sobie okiem tutaj. Tu chodzi oto, że są jakieś listenery na eventach i tak to powinno być zrobione. Może nie trzeba w twoim przypadku iść aż do redisa itp. można by z jakieś klasy ogarniającej eventy skorzystać.
Comment on lines
+57
to
+90
| def __dict__(self) -> dict: | ||
| """Dictionary representation. | ||
|
|
||
| Returns: | ||
| dict: Dictionary representation. | ||
| """ | ||
| return { | ||
| "id": self.sched_job.id, | ||
| "cron": ", ".join(f"{x.name}: {str(x)}" for x in self.trigger.fields), | ||
| "timezone": self.trigger.timezone, | ||
| "next_run": timestamp_to_human_readable(self.trigger.get_next_fire_time(datetime.now(), datetime.now()).timestamp()) | ||
| } | ||
|
|
||
| @property | ||
| def logger(self) -> logging.Logger: | ||
| """The logger property. | ||
|
|
||
| Returns: | ||
| logging.Logger: The logger instance | ||
| """ | ||
| return self._logger | ||
|
|
||
| @logger.setter | ||
| def logger(self, logger:logging.Logger) -> None: | ||
| """Set the logger. | ||
|
|
||
| Args: | ||
| logger (logging.Logger): Logger. | ||
| """ | ||
| if logger is None: | ||
| logging.config.fileConfig("log_dev.conf") | ||
| self._logger = logging.getLogger('pybackupper_logger') | ||
| else: | ||
| self._logger = logger |
Comment on lines
+194
to
+214
| if day_of_week == "*": | ||
| return CronTrigger( | ||
| minute=minute, | ||
| hour=hour, | ||
| day=day_of_month, | ||
| month=month, | ||
| timezone=timezone) | ||
|
|
||
| day_of_week = day_of_week.lower().split(",") | ||
|
|
||
| for i, day in enumerate(day_of_week): | ||
| if day not in ["mon", "tue", "wed", "thu", "fri", "sat", "sun"]: | ||
| try: | ||
| i_day = int(day) | ||
| except ValueError as e: | ||
| raise ValueError("Invalid days of week") from e | ||
|
|
||
| if i_day < 0 or i_day > 6: | ||
| raise ValueError("Invalid days of week") | ||
|
|
||
| day_of_week[i] = ["mon", "tue", "wed", "thu", "fri", "sat", "sun"][i_day] |
halicki
marked this pull request as ready for review
March 6, 2025 13:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hej! Tworzę tego pull requesta, żeby mieć na czym pisaś komentarze.