Skip to content

Conversation

topiaruss
Copy link

If a working app is plopped into a container, there is no .git dir, so the Repo() access raises an exception.
This prints a minimal error message, and a clue to the UI why the commit may not show.

@topiaruss topiaruss closed this Nov 15, 2021
@topiaruss topiaruss reopened this Nov 15, 2021
@topiaruss
Copy link
Author

Would be great to close my fork.

Copy link
Member

@Xiphoseer Xiphoseer left a comment

Choose a reason for hiding this comment

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

This is a good suggestion, thank you! Added two small comments otherwise this seems ready to merge.

repo.__del__()
return chash
except Exception:
print(f"git_version - trouble with repo path {path}")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print(f"git_version - trouble with repo path {path}")
logger.debug(f"Failed to find current commit for {path}")

Can you replace the print statement with a logger call as described in https://docs.djangoproject.com/en/3.2/topics/logging/#using-logging? That's probably more appropriate for something published as a library.

Just for the sake of completeness, this would also need

import logging
logger = logging.getLogger(__name__)

somewhere at the start of the file

chash = commit.hexsha
repo.__del__()
return chash
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
except Exception:
except Exception as e:

Is there a specific reason not to print the actual exception message? Was that too verbose?

@topiaruss
Copy link
Author

Yesterday spotted your message, and updated as requested.
Also gave version headroom for django 4.x.x.

@topiaruss
Copy link
Author

bump

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants