Skip to content

Dump: add output format tar and output to stdout #10376

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
Merged
merged 15 commits into from
Jun 5, 2020
Merged

Dump: add output format tar and output to stdout #10376

merged 15 commits into from
Jun 5, 2020

Conversation

PhilippHomann
Copy link
Contributor

To read dumps output from docker exec I needed support for tar and write to stdout.

PhilippHomann
PhilippHomann commented Feb 20, 2020
View reviewed changes
cmd/dump.go Outdated
fileName := ctx.String("file")
if fileName == "-" {
file = os.Stdout
log.DelLogger("console")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't found a better way to setup the console logger to log to stderr.
So I just deleted the logger as logs may break the file written to stdout.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 20, 2020
@lunny lunny added the type/enhancement An improvement of existing functionality label Feb 20, 2020
@PhilippHomann PhilippHomann requested a review from lunny February 25, 2020 09:12
@lunny
Copy link
Member

lunny commented Feb 25, 2020

@PhilippHomann CI failed.

@PhilippHomann
Copy link
Contributor Author

Okay, I see.
But the problematic part is the one I mentioned above.
Is there any way to setup the console logger to log to stderr?
If so I would be happy to implement it or even fix the missing return code verification.

zeripath
zeripath reviewed Feb 28, 2020
View reviewed changes
zeripath
zeripath reviewed Feb 28, 2020
View reviewed changes
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@94f2951). Click here to learn what that means.
The diff coverage is 71.64%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #10376   +/-   ##
=========================================
  Coverage          ?   43.69%           
=========================================
  Files             ?      586           
  Lines             ?    81879           
  Branches          ?        0           
=========================================
  Hits              ?    35775           
  Misses            ?    41661           
  Partials          ?     4443
Impacted Files Coverage Δ
models/issue_label.go 63.04% <ø> (ø)
modules/structs/user_app.go 0% <ø> (ø)
modules/indexer/code/indexer.go 38.02% <10%> (ø)
modules/convert/pull.go 71.07% <100%> (ø)
modules/convert/convert.go 75.88% <100%> (ø)
modules/markup/sanitizer.go 92.2% <100%> (ø)
routers/api/v1/api.go 76.13% <100%> (ø)
models/issue_assignees.go 63.9% <100%> (ø)
routers/api/v1/repo/label.go 83.71% <100%> (ø)
modules/indexer/issues/indexer.go 56.19% <14.28%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94f2951...5f1fa32. Read the comment docs.

zeripath
zeripath reviewed Feb 29, 2020
View reviewed changes
zeripath
zeripath approved these changes Feb 29, 2020
View reviewed changes
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Small nit with CreateFile - i.e. create the file with the mode 0600 initially rather than restrict later.

Otherwise this seems OK.

(The number of dependencies did concern me but these seem reasonable.)

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 29, 2020
@PhilippHomann PhilippHomann requested a review from zeripath March 5, 2020 08:15
@PhilippHomann
Copy link
Contributor Author

@zeripath Sorry for requesting new approval. Accidentally clicked the button ...
@lunny What do you think?

@PhilippHomann PhilippHomann mentioned this pull request Apr 20, 2020
Closed
7 tasks
@lafriks
Copy link
Member

lafriks commented Apr 27, 2020

please resolve conflicts

@PhilippHomann PhilippHomann reopened this Apr 28, 2020
@PhilippHomann
Copy link
Contributor Author

@lafriks Did some refactoring and pushed, but seems like drone crashed. Could you please trigger a rebuild?

@6543
Copy link
Member

6543 commented Apr 28, 2020

@PhilippHomann you need go v1.14 now

pleace execute make vendor on a system with Go1.14 and commit the change :)

@PhilippHomann PhilippHomann reopened this Apr 29, 2020
@PhilippHomann
Copy link
Contributor Author

@lafriks Got it finally.

@PhilippHomann PhilippHomann mentioned this pull request May 7, 2020
Closed
7 tasks
@PhilippHomann
Dump: Use mholt/archive/v3 to support tar including many compressions
d7bd7a7 
Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>
@PhilippHomann
Dump: Fixed bug present since #6677 where SessionConfig.Provider is n…
d505f68 
…ever "file"

Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>
@PhilippHomann
Dump: never pack RepoRootPath, LFS.ContentPath and LogRootPath when t…
a00ac3f 
…hey are below AppDataPath

Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>
@PhilippHomann
Dump: also dump LFS (fixes #10058)
359d94c 
Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>
@PhilippHomann
Dump: never dump CustomPath if CustomPath is a subdir of or equal to …
f1809ad 
…AppDataPath (fixes #10365)

Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>
PhilippHomann and others added 2 commits May 7, 2020 13:53
@PhilippHomann
Dump: Allow dump output to stdout
41b1950 
Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>
@zeripath
Merge branch 'master' into master
e23b164
zeripath
zeripath reviewed May 7, 2020
View reviewed changes
@codecov-commenter
Copy link

codecov-commenter commented May 21, 2020 •
edited
Loading

Codecov Report

Merging #10376 into master will decrease coverage by 0.17%.
The diff coverage is 1.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10376      +/-   ##
==========================================
- Coverage   43.95%   43.78%   -0.18%     
==========================================
  Files         614      607       -7     
  Lines       87428    87031     -397     
==========================================
- Hits        38431    38104     -327     
+ Misses      44259    44220      -39     
+ Partials     4738     4707      -31
Impacted Files Coverage Δ
modules/setting/setting.go 45.11% <ø> (-0.43%) ⬇️
cmd/dump.go 0.83% <1.21%> (+0.83%) ⬆️
modules/indexer/stats/db.go 40.62% <0.00%> (-18.75%) ⬇️
modules/indexer/stats/queue.go 62.50% <0.00%> (-18.75%) ⬇️
routers/api/v1/org/team.go 37.12% <0.00%> (-10.04%) ⬇️
modules/git/utils.go 65.67% <0.00%> (-9.33%) ⬇️
services/issue/issue.go 23.68% <0.00%> (-8.09%) ⬇️
models/issue_tracked_time.go 58.41% <0.00%> (-6.12%) ⬇️
routers/repo/compare.go 41.47% <0.00%> (-3.96%) ⬇️
modules/git/repo.go 47.28% <0.00%> (-3.35%) ⬇️
... and 56 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02a52d6...758dc70. Read the comment docs.

@6543
Copy link
Member

6543 commented May 21, 2020

pleace resolve conflict

@PhilippHomann
Copy link
Contributor Author

@6543 Done but don't know what happened in drone..
Local build works, so seems to be an internal error as the log states.

@PhilippHomann
Copy link
Contributor Author

@6543 Build worked now. Could you please give your approval?

@6543
Copy link
Member

6543 commented May 26, 2020

Could you please give your approval?

I first have to look through the code

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 5, 2020
@techknowlogick techknowlogick merged commit 684b7a9 into go-gitea:master Jun 5, 2020
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
@PhilippHomann @zeripath @techknowlogick
Dump: add output format tar and output to stdout (go-gitea#10376)
40bd1d5 
* Dump: Use mholt/archive/v3 to support tar including many compressions

Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>

* Dump: Allow dump output to stdout

Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>

* Dump: Fixed bug present since go-gitea#6677 where SessionConfig.Provider is never "file"

Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>

* Dump: never pack RepoRootPath, LFS.ContentPath and LogRootPath when they are below AppDataPath

Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>

* Dump: also dump LFS (fixes go-gitea#10058)

Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>

* Dump: never dump CustomPath if CustomPath is a subdir of or equal to AppDataPath (fixes go-gitea#10365)

Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>

* Use log.Info instead of fmt.Fprintf

Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>

* import ordering

* make fmt

Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Co-authored-by: Matti R <matti@mdranta.net>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@techknowlogick techknowlogick techknowlogick approved these changes

@lunny lunny Awaiting requested review from lunny

+1 more reviewer

@zeripath zeripath zeripath approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@PhilippHomann @lunny @codecov-io @lafriks @6543 @codecov-commenter @techknowlogick @zeripath @GiteaBot