Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prefix subvolume paths with a / everywhere, #41

Merged
merged 13 commits into from
Dec 28, 2016
Merged

Prefix subvolume paths with a / everywhere, #41

merged 13 commits into from
Dec 28, 2016

Conversation

florianjacob
Copy link
Collaborator

by which it was possible to generalize mountpoint handling
and exclude the mountpoint as subvolume to snapshot if wanted.

Resolves #38.

@florianjacob florianjacob mentioned this pull request Oct 28, 2016
Closed
@florianjacob
Copy link
Collaborator Author

florianjacob commented Oct 28, 2016 •
edited
Loading

What is left to do:

  • Update markdown docs, I could not find the required perl module to generate them on Arch Linux.
  • Fix the failing tests.
  • Expand the tests for the new functionality.
  • If the exclude file has errors, the SYNTAX ERROR message comes up twice. Probably my way of exiting does not really exit immediately?

I don't know how to continue from here. @csirac2 could you help me with those points? Feel free to just push new commits to the branch.

I started to use this branch now for my purposes and see how it turns out in pratice.

@csirac2
Copy link
Owner

csirac2 commented Oct 28, 2016

Happy to help, will take a look next weekend :)

@florianjacob
Copy link
Collaborator Author

Thanks! I'll report whether my code set my backups on fire in the meantime. 😉

@florianjacob
Copy link
Collaborator Author

I think I managed to fix the tests with their now helpful output, but travis doesn't manage to build stuff today, I guess I'll try again tomorrow.

csirac2
csirac2 reviewed Nov 18, 2016
View reviewed changes
snazzer
glob2grep_file() {
FILE=$1
OUT=$(mktemp)

# check if every line starts with either / or *, if not it's a syntax error.
while read -r line; do
if ! [[ "$line" =~ ^[/\*].* ]]; then
Copy link
Owner

Choose a reason for hiding this comment

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

This is failing because [[ =~ is a bash-ism, but we are a #/bin/sh script ;-)

Perhaps try something like

if ! grep '^[/\*]' "$1"; then
    echo "SYNTAX ERROR: $1 contains lines that start with neither / nor *." >&2
    exit 12
fi

csirac2
csirac2 requested changes Nov 18, 2016
View reviewed changes
Copy link
Owner

@csirac2 csirac2 left a comment

Choose a reason for hiding this comment

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

The tests should pass on Travis once we remove the [[ =~ "bash-isms" - see comment

@florianjacob florianjacob force-pushed the prefix-slash branch 2 times, most recently from 794aeec to be20ad1 Compare November 18, 2016 23:57
@florianjacob
Copy link
Collaborator Author

florianjacob commented Nov 19, 2016 •
edited
Loading

I now settled with

if ! echo "$line" | grep '^[/*]' >/dev/null; then

which really seems to work! Just a little headbanging on strange erors like „grep cannot be found“ was required to get to the echo and >/dev/null redirection. ;)

Should have run shellcheck by myself to rule this out, sorry. 😆

@florianjacob
Copy link
Collaborator Author

Regarding the doubled SYNTAX ERROR, I now understand why it's happening, the functions which use glob2grep_file use it as subprocesses and don't check an exit value from glob2grep, so they just continue, as well as their processes… 😞

Is there a nicer way to do this than ripple the exit status up until the parent snazzer process?
One could separate the syntax checking part and call it directly from the main snazzer process, but then I'd need to do that only in cases where having the file is actually relevant…

@florianjacob
Copy link
Collaborator Author

Found a way to cleanly fix the SYNTAX ERROR problem, but now the tests fail again. :/ investigating…
Does not seem to be a bashism this time, seems like this has somehow changed exclude behaviour.

@florianjacob
Copy link
Collaborator Author

hooray, things fixed. 😄

csirac2
csirac2 approved these changes Dec 13, 2016
View reviewed changes
Copy link
Owner

@csirac2 csirac2 left a comment

Choose a reason for hiding this comment

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

Great work!

@csirac2
Copy link
Owner

csirac2 commented Dec 13, 2016

Whoops! I noticed after I accepted the merge that it's still WIP... let me know if it really wasn't ready yet

@florianjacob
Copy link
Collaborator Author

I still want to extend the tests a little to cover more of this new functionality.

Other than that, doing a 0.1 release of the master branch before merging this is the only thing left to do. 😉 Is there anything you want in the 0.1 release that's not already in, or can I start with release preparation (on a different branch)?

@csirac2
Copy link
Owner

csirac2 commented Dec 15, 2016

I've just done some gardening of the milestones here, please assign issues you want released in 0.1 to the 0.1 release milestone: https://github.com/csirac2/snazzer/milestones

The only one I really had my heart set on was #22 - I've moved some of the annoying bugs to "0.6", these were bugs had planned to have fixed in a first release but I don't want them to stop you doing the important work of rolling a first release :)

I would like to see a TODO or ISSUES.txt in the release with the unresolved issues listed. Perhaps I can work on that and #22 this weekend.

@florianjacob
Copy link
Collaborator Author

I'd prefer to have this in 0.1, as this might attract more people to using snazzer. Also, #47 will be easier to work out with this included.

But I'm not sure, just breaking the old config files without at least releasing a version… maybe we should tag the current state of the git repository as some kind of 0.0 or 0.0.1 (pre-)release? What do you think?

@csirac2
Copy link
Owner

csirac2 commented Dec 21, 2016

Yes, we should tag before merging this in. 0.0.1 is fine. Please feel free to use the milestone feature to assign this PR to 0.1

@florianjacob florianjacob self-assigned this Dec 22, 2016
@florianjacob florianjacob added this to the 0.1 milestone Dec 22, 2016
@florianjacob florianjacob changed the title WIP: Prefix subvolume paths with a / everywhere, Prefix subvolume paths with a / everywhere, Dec 27, 2016
@florianjacob
Copy link
Collaborator Author

florianjacob commented Dec 27, 2016 •
edited
Loading

Finished! 😄 This is everything I wanted to do in this PR. From my side, this could be merged after #51 was completed.

@csirac2
Copy link
Owner

csirac2 commented Dec 28, 2016

Excellent work! Just made a one-char typo fix. Happy to accept this once #51 is merged.

I'd better get my /etc/snazzer.conf PR done or I'll miss the release window :D

@florianjacob
Copy link
Collaborator Author

Thanks! 😉

@florianjacob
Prefix subvolume paths with a / everywhere,
afae4b8 
by which it was possible to generalize mountpoint handling
and exclude the mountpoint as subvolume to snapshot if wanted.
@florianjacob
fit tests to new slash handling
3b151bb
@florianjacob florianjacob merged commit 34a60e0 into master Dec 28, 2016
@florianjacob florianjacob deleted the prefix-slash branch December 28, 2016 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csirac2 csirac2 csirac2 approved these changes

Assignees

@florianjacob florianjacob

Labels
None yet
Projects
None yet
Milestone
0.1
Development

Successfully merging this pull request may close these issues.
2 participants
@florianjacob @csirac2