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

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

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.

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.

@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.

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?

@lafriks
Copy link
Member

lafriks commented Apr 27, 2020

please resolve conflicts

@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
Copy link
Contributor Author

@lafriks Got it finally.

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2020

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
* Dump: Use mholt/archive/v3 to support tar including many compressions

Signed-off-by: Philipp Homann <[email protected]>

* Dump: Allow dump output to stdout

Signed-off-by: Philipp Homann <[email protected]>

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

Signed-off-by: Philipp Homann <[email protected]>

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

Signed-off-by: Philipp Homann <[email protected]>

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

Signed-off-by: Philipp Homann <[email protected]>

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

Signed-off-by: Philipp Homann <[email protected]>

* Use log.Info instead of fmt.Fprintf

Signed-off-by: Philipp Homann <[email protected]>

* import ordering

* make fmt

Co-authored-by: zeripath <[email protected]>
Co-authored-by: techknowlogick <[email protected]>
Co-authored-by: Matti R <[email protected]>
@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.
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
Development

Successfully merging this pull request may close these issues.

9 participants