borgmatic-collective/borgmatic
borgmatic-collective/borgmatic
15
61
Fork 44
Code Issues 57 Pull Requests 1 Actions Packages Releases 124 Activity

Fix importlib.metadata.files workaround #702

Merged
witten merged 4 commits from kaechele/borgmatic:fix-importlib-files into main 2023-06-04 05:04:41 +00:00
Conversation 5 Commits 4 Files Changed 2 +19 -27
kaechele commented 2023-05-23 21:27:27 +00:00
Contributor
Copy Link

Some distributions, such as Fedora, do not install the RECORDS file as
part of a package's dist-info. As a result importlib.metadata.files will
return None.

Use the workaround for these cases as well.

Signed-off-by: Felix Kaechele felix@kaechele.ca

Some distributions, such as Fedora, do not install the RECORDS file as part of a package's dist-info. As a result importlib.metadata.files will return None. Use the workaround for these cases as well. Signed-off-by: Felix Kaechele <felix@kaechele.ca>
witten reviewed 2023-05-23 21:56:32 +00:00
witten left a comment
Owner
Copy Link

Thanks for taking the time to fix this!

A couple of thoughts:

  • Given the complexity/unreliability of the importlib_metadata approach, what do you think of ditching that whole code path and doing the os.path.join(os.path.dirname(borgmatic.config.__file__), 'schema.yaml') thing unconditionally? Does that seem like it'd work in all cases?
  • With these changes, I'd appreciate if you also updated / added to the the existing unit tests for this function in tests/unit/config/test_validate.py.
Thanks for taking the time to fix this! A couple of thoughts: * Given the complexity/unreliability of the `importlib_metadata` approach, what do you think of ditching that whole code path and doing the `os.path.join(os.path.dirname(borgmatic.config.__file__), 'schema.yaml')` thing unconditionally? Does that seem like it'd work in all cases? * With these changes, I'd appreciate if you also updated / added to the the existing unit tests for this function in `tests/unit/config/test_validate.py`.
kaechele commented 2023-05-25 00:39:45 +00:00
Author
Contributor
Copy Link

Sure thing!

I think it makes sense to just determine the location of the schema based on the config.__file__. As long as the Python packaging tooling installs it alongside that file (and so far it does) we should be good.

I'll take another look at it and amend the pull request accordingly.

Sure thing! I think it makes sense to just determine the location of the schema based on the `config.__file__`. As long as the Python packaging tooling installs it alongside that file (and so far it does) we should be good. I'll take another look at it and amend the pull request accordingly.
๐Ÿ‘ 1
witten commented 2023-06-01 16:36:47 +00:00
Owner
Copy Link

I was hoping to get this fix into the next release. If you won't have a chance to work on this soon, just let me know and I can pick it up from here. Thank you!

I was hoping to get this fix into the next release. If you won't have a chance to work on this soon, just let me know and I can pick it up from here. Thank you!
kaechele force-pushed fix-importlib-files from b316ccbdc5 to 5c6a61fd14 2023-06-03 01:43:07 +00:00 Compare
kaechele force-pushed fix-importlib-files from 5c6a61fd14 to 8d5cc53813 2023-06-03 01:44:46 +00:00 Compare
kaechele commented 2023-06-03 01:49:02 +00:00
Author
Contributor
Copy Link

Alright. I've updated my branch. Somehow managed to get it out of sync, hence the force push. Sorry about that. I usually prefer working on branches iteratively and have them squashed into one commit that gets merged at the end.

One thing I was wondering is whether we should validate that a file exists at that path so that we can raise FileNotFoundError from that function if required (which seems to have been the original behaviour in the first place).

Alright. I've updated my branch. Somehow managed to get it out of sync, hence the force push. Sorry about that. I usually prefer working on branches iteratively and have them squashed into one commit that gets merged at the end. One thing I was wondering is whether we should validate that a file exists at that path so that we can raise FileNotFoundError from that function if required (which seems to have been the original behaviour in the first place).
witten commented 2023-06-03 06:28:19 +00:00
Owner
Copy Link

Alright. I've updated my branch. Somehow managed to get it out of sync, hence the force push. Sorry about that. I usually prefer working on branches iteratively and have them squashed into one commit that gets merged at the end.

No worries! (I'm generally pretty loosey-goosey about my own commits on this project.)

One thing I was wondering is whether we should validate that a file exists at that path so that we can raise FileNotFoundError from that function if required (which seems to have been the original behaviour in the first place).

Yes, that seems reasonable. Thank you!

> Alright. I've updated my branch. Somehow managed to get it out of sync, hence the force push. Sorry about that. I usually prefer working on branches iteratively and have them squashed into one commit that gets merged at the end. No worries! (I'm generally pretty loosey-goosey about my own commits on this project.) > One thing I was wondering is whether we should validate that a file exists at that path so that we can raise FileNotFoundError from that function if required (which seems to have been the original behaviour in the first place). Yes, that seems reasonable. Thank you!
witten requested changes 2023-06-04 02:47:15 +00:00
witten left a comment
Owner
Copy Link

Thanks for doing another round on this! I left some annoying, nit-picky comments on some of the particulars in this PR. If at any point you get tired of the back-and-forth, please let me know and I'd be happy to finish this up.

Thanks for doing another round on this! I left some annoying, nit-picky comments on some of the particulars in this PR. If at any point you get tired of the back-and-forth, please let me know and I'd be happy to finish this up.
borgmatic/config/validate.py Outdated
@ -31,1 +17,3 @@
return os.path.join(os.path.dirname(borgmatic.config.__file__), 'schema.yaml')
schema_path = os.path.join(os.path.dirname(borgmatic.config.__file__), 'schema.yaml')
if os.path.exists(schema_path) and os.path.isfile(schema_path):
witten commented 2023-06-04 02:39:25 +00:00
Owner
Copy Link

Turns out you don't need exists() if you've also got isfile()!

Turns out you don't need `exists()` if you've also got `isfile()`!
kaechele marked this conversation as resolved
borgmatic/config/validate.py Outdated
@ -32,0 +19,4 @@
if os.path.exists(schema_path) and os.path.isfile(schema_path):
return schema_path
raise FileNotFoundError
witten commented 2023-06-04 02:41:28 +00:00
Owner
Copy Link

Just raising the exception type like this means that it won't include the filename in the exception, and therefore any error messages won't tell the user what file wasn't found. One way around that is to set the message manually when you raise the error type. But an even easier way might be to actually open() the file here. Then you won't need the exists()/isfile() checks and it'll conveniently raise a FileNotFoundError populated with an appropriate filename message.

Just raising the exception type like this means that it won't include the filename in the exception, and therefore any error messages won't tell the user what file wasn't found. One way around that is to set the message manually when you raise the error type. But an even easier way might be to actually `open()` the file here. Then you won't need the `exists()`/`isfile()` checks *and* it'll conveniently raise a `FileNotFoundError` populated with an appropriate filename message.
kaechele commented 2023-06-04 04:56:34 +00:00
Author
Contributor
Copy Link

Went with the open() approach.

Went with the `open()` approach.
tests/unit/config/test_validate.py Outdated
@ -23,3 +8,1 @@
)
assert module.schema_filename().endswith('/borgmatic/config/schema.yaml')
module.schema_filename().endswith('/borgmatic/config/schema.yaml')
witten commented 2023-06-04 02:44:55 +00:00
Owner
Copy Link

Now that the unit under test is calling exists()/isfile() (or open() as suggested above), those functions need to be mocked out here IMO. Otherwise, you run the risk of the environment in which the tests are running impacting the tests. (I realize schema.yaml should always be present.)

Additionally, now that you've got an if in your unit under test, ideally that code path would be tested as well. Which you could do with a separate test function and judicious use of mocks.

Now that the unit under test is calling `exists()`/`isfile()` (or `open()` as suggested above), those functions need to be mocked out here IMO. Otherwise, you run the risk of the environment in which the tests are running impacting the tests. (I realize `schema.yaml` *should* always be present.) Additionally, now that you've got an `if` in your unit under test, ideally that code path would be tested as well. Which you could do with a separate test function and judicious use of mocks.
kaechele commented 2023-06-04 04:57:16 +00:00
Author
Contributor
Copy Link

I've updated the tests and am getting 100% coverage.

I've updated the tests and am getting 100% coverage.
kaechele force-pushed fix-importlib-files from deaa6a9c6f to c61d63b235 2023-06-04 04:55:51 +00:00 Compare
kaechele commented 2023-06-04 04:58:09 +00:00
Author
Contributor
Copy Link

I actually appreciate nitpicky comments about my PRs as an opportunity to learn. Thanks for taking the time.

I actually appreciate nitpicky comments about my PRs as an opportunity to learn. Thanks for taking the time.
witten approved these changes 2023-06-04 05:02:40 +00:00
witten left a comment
Owner
Copy Link

Awesome, thank you for making these changes!

Awesome, thank you for making these changes!
tests/unit/config/test_validate.py
@ -13,3 +14,1 @@
flexmock(match=lambda path: False, locate=lambda: None),
)
flexmock(os.path).should_receive('dirname').and_return("/var/borgmatic/config")
witten commented 2023-06-04 05:02:13 +00:00
Owner
Copy Link

Another nit: This codebase uses single quotes for strings, but I can make this change after merging.

Another nit: This codebase uses single quotes for strings, but I can make this change after merging.
kaechele commented 2023-06-04 05:06:35 +00:00
Author
Contributor
Copy Link

Ah, yes. I trusted a bit too much in black to do its thing ;-)

Ah, yes. I trusted a bit too much in `black` to do its thing ;-)
witten commented 2023-06-04 05:07:06 +00:00
Owner
Copy Link

No worries. I do that all the time. ๐Ÿ˜„

No worries. I do that all the time. ๐Ÿ˜„
witten merged commit b3f70434df into main 2023-06-04 05:04:41 +00:00
kaechele deleted branch fix-importlib-files 2023-06-04 05:16:42 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
witten
Labels
Clear labels   
bug

   
data loss

   
design finalized

   
good first issue

   
new feature area

   
question / support

   
security

   
waiting for response
No Label
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#702
No description provided.
Delete Branch "kaechele/borgmatic:fix-importlib-files"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?