Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Kdb mount #1745

Merged
merged 6 commits into from
Dec 21, 2017
Merged

Kdb mount #1745

merged 6 commits into from
Dec 21, 2017

Conversation

markus2330
Copy link
Contributor

@markus2330 markus2330 commented Dec 19, 2017

Adds mount/dini features as discussed in #1693 and #1306

Checklist

Please only check relevant points.
For docu fixes, spell checking and similar nothing
needs to be checked.

  • commit messages are fine (with references to issues)
  • I added unit tests
  • I ran all tests and everything went fine
  • affected documentation is fixed (partly, not for strategy)
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins)
  • release notes are updated (doc/news/_preparation_next_release.md)

@sanssecours please review my pull request

Markus Raab added 5 commits December 19, 2017 19:02
to not fail on existing mountpoints
"--strategy unchanged" as discussed #1306
seems to be buggy:
e.g. if plugins add plugin config (like dump)
     parent keys disappear and it seems like
     the config was changed (although it was not)
@markus2330
Copy link
Contributor Author

jenkins build all please

Copy link
Member

@sanssecours sanssecours left a comment

Choose a reason for hiding this comment

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

Thank you for fixing issue #1306. Everything looks good as far as I can tell. I would prefer that mounting an already existing mountpoint (with the same configuration) would just work without adding the additional switch -f though. For example, I would expect the following two commands

kdb mount config.ini /examples/ini ini
kdb mount config.ini /examples/ini ini

to not cause any problems at all. That might just be my preference though.

One thing I also noticed was that there is no (Markdown Shell Recorder) test for the new behavior of kdb mount. It would be very nice if you could also add a test for kdb mount -f.

@markus2330
Copy link
Contributor Author

to not cause any problems at all. That might just be my preference though.

Yes, I see it the same way. The problem is that it does not always work smoothly (for cases where plugins modify their config and INI is unable to store parent dirs). Thus I would prefer to have it as a separate switch until we get a reliable behavior.

Other issues (like INI crashes) are imho more important for this release.

@markus2330 markus2330 merged commit 41d3bc9 into master Dec 21, 2017
@markus2330 markus2330 deleted the kdb-mount branch December 21, 2017 12:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants