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.

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

Add --append-only to borg serve (1.0-maint) #1229

Merged
merged 1 commit into from
Jun 30, 2016

Conversation

PlasmaPower
Copy link
Contributor

@PlasmaPower PlasmaPower commented Jun 30, 2016

Replaces #1228 which resolves #1168

@enkore
Copy link
Contributor

enkore commented Jun 30, 2016

I don't see any existing tests for borg serve, is that correct?

The RPC / serve part is tested via RemoteArchiverTestCase in borg.testsuite.archiver and the RemoteXXX tests in borg.testsuite.repository. An extension of RepositoryAppendOnlyTestCase (borg.testsuite.repository) would be sufficient (cf. RemoteRepositoryTestCase).

@PlasmaPower
Copy link
Contributor Author

@enkore Great, makes sense! Thanks for the help.

@ThomasWaldmann
Copy link
Member

Hmm, doesn't it make sense to test local and remote? The cmdline option could also be used for locally mounted stuff, maybe for slightly different reasons.

@PlasmaPower
Copy link
Contributor Author

How's that latest commit for a start? Now I just need to test that --append-only will get translated into a repo created with that flag.

Also, I keep almost pushing my .eggs, is there any way to fix that? Shouldn't it be in the .gitignore?

@enkore
Copy link
Contributor

enkore commented Jun 30, 2016

Also, I keep almost pushing my .eggs, is there any way to fix that? Shouldn't it be in the .gitignore?

build/dist/borg.build/.dist etc. are all in the git ignore...?

@PlasmaPower
Copy link
Contributor Author

$ git status
On branch serve-append-only
Your branch is up-to-date with 'origin/serve-append-only'.
Untracked files:
  (use "git add <file>..." to include in what will be committed)

        .eggs/

nothing added to commit but untracked files present (use "git add" to track)

$ cat .eggs/README.txt
This directory contains eggs that were downloaded by setuptools to build, test, and run plug-ins.

This directory caches those eggs to prevent repeated downloads.

However, it is safe to delete this directory.

@enkore
Copy link
Contributor

enkore commented Jun 30, 2016

Oi, didn't notice that. But yeah, they're here as well, it's just that my worktrees are littered with stuff.

Guess setup.py develop does that, since pip/tox don't use eggs afaik. Suppose I should use pip install -e . instead.

@PlasmaPower
Copy link
Contributor Author

I think that's all the testing that should be done with the current testing infrastructure. Anything more? If not, I think this is ready to merge.

@PlasmaPower PlasmaPower changed the title Add --append-only to borg serve (WIP - 1.0-maint) Add --append-only to borg serve (1.0-maint) Jun 30, 2016
@ThomasWaldmann
Copy link
Member

Search where the docs refer to append-only and add some words and an example how it can be used after your change.

@PlasmaPower
Copy link
Contributor Author

Right, docs. Forgot about those. Should be good with the latest commit.

@@ -727,6 +727,13 @@ To activate append-only mode, edit the repository ``config`` file and add a line
In append-only mode Borg will create a transaction log in the ``transactions`` file,
where each line is a transaction and a UTC timestamp.

In addition, ``borg serve`` can act as if a repository is in append-only mode with
it's option ``--append-only``. This can be very useful for fine-tuning access control
Copy link
Member

Choose a reason for hiding this comment

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

typo: "its"

@ThomasWaldmann
Copy link
Member

let's see if github finds it also in the 2nd line.

@PlasmaPower
Copy link
Contributor Author

Yep!

PlasmaPower added a commit to PlasmaPower/borg that referenced this issue 3 minutes ago
@PlasmaPower Add --append-only to borg serve

Also, I know it does from experience.

@enkore
Copy link
Contributor

enkore commented Jun 30, 2016

LGTM, merge after travis?

@ThomasWaldmann
Copy link
Member

yes.

@PlasmaPower thanks for contributing. Visit bountysource after the issue is closed and claim.

also, feel free to add yourself to AUTHORS file.

@PlasmaPower
Copy link
Contributor Author

Side note on fixing an issue in the commit message: if you do that from the beginning and you amend to the commit every amendment will stay on the issue page which clogs it up fast.

@ThomasWaldmann Glad I could help! I'll be looking to contribute more in the future, even on issues without bounties. This project is a lot better than just rsync, I'll be personally using it.

@PlasmaPower
Copy link
Contributor Author

PlasmaPower commented Jun 30, 2016

I made a linting mistake in the repository tests. Fixed, but Travis will have to run again.

@ThomasWaldmann
Copy link
Member

@PlasmaPower ah, interesting note about the commit msg.

@codecov-io
Copy link

codecov-io commented Jun 30, 2016

Current coverage is 82.64%

Merging #1229 into 1.0-maint will increase coverage by 0.01%

@@           1.0-maint      #1229   diff @@
===========================================
  Files             14         14          
  Lines           4243       4246     +3   
  Methods            0          0          
  Messages           0          0          
  Branches         745        745          
===========================================
+ Hits            3506       3509     +3   
  Misses           521        521          
  Partials         216        216          

Sunburst

Powered by Codecov. Last updated by 1242653...79df1bd

@PlasmaPower
Copy link
Contributor Author

Travis is done, it passed.

@ThomasWaldmann ThomasWaldmann merged commit 09f184c into borgbackup:1.0-maint Jun 30, 2016
@ThomasWaldmann
Copy link
Member

@PlasmaPower you committed .egg stuff. We completely killed that from history, please try again.

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.

4 participants