Skip to content

Conversation

@glaszig
Copy link
Contributor

@glaszig glaszig commented Mar 30, 2019

what

with this patch you can submit a custom nam for the backup file created by gitea dump.

gitea dump -f gitea-backup.zip

why

without this patch, when writing a backup script which stores my gitea dump off-site, i have to do some weird gymnastics to get the file gitea dump writes to disk because it generates a random file name with the current unix timestamp. like dump_file=$(ls -t|head -1) while being sure to be in an empty directory.

with this patch, i can just choose the file's name myself and move on encrypting and uploading it.

compatibility

this change should not affect anybody as the new parameter is optional and has a default value which does not change the known behavior of the binary.

@codecov-io
Copy link

codecov-io commented Mar 30, 2019

Codecov Report

Merging #6474 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #6474   +/-   ##
======================================
  Coverage    39.4%   39.4%           
======================================
  Files         393     393           
  Lines       53271   53271           
======================================
  Hits        20994   20994           
  Misses      29290   29290           
  Partials     2987    2987

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 2e1ead8...278bfe1. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 30, 2019
cmd/dump.go Outdated
Copy link
Member

@adelowo adelowo Mar 30, 2019

Choose a reason for hiding this comment

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

Also if the value of --file is provided by the user and doesn't have a %d format directive, wouldn't that ruin the value of fileName. If I remember correctly, when a string doesn't have a format directive, the resulting output is something like %!(EXTRA int64=1257894000). So in this case, fileName will end up being userSuppliedFileName%!(EXTRA int64=1257894000) .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you, sir, are completely right. updated the logic.

@jolheiser
Copy link
Member

jolheiser commented Mar 31, 2019

In my opinion, the default should remain the same with the timestamp, and the override should just override without giving the %d timestamp option.

If a user wants to override the filename, they should provide the exact name they want. If they need a timestamp, they can figure out how to generate it via shell (or whatever terminal they use per their OS).

@glaszig
Copy link
Contributor Author

glaszig commented Mar 31, 2019 via email

@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 Mar 31, 2019
@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 Mar 31, 2019
cmd/dump.go Outdated
fmtString := ctx.String("file")

if strings.Count(fmtString, "%d") > 0 {
fmtString = fmt.Sprintf(fmtString, time.Now().Unix())
Copy link
Member

@jolheiser jolheiser Apr 1, 2019

Choose a reason for hiding this comment

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

In reference to my other comment, personally I think this is unnecessary.

If a user wants to override the file name, they should be able to provide the exact name they want, without relying on the code transforming a %d. This code would only transform a single %d as well, and if they had any other formatting symbols before/after, they would also end up with a bad filename.

I will not block this PR, however I don't personally approve of this "magic".
(To clarify, I do like this feature. Overriding the filename would be great. I just don't particularly care for this part.)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the "dupe" comment notification, I forget that comments in the file section don't show up in conversation unless it's marked as a review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i see your point and generally agree. but if you don't submit a custom file name with the --file argument, it'll use the default and the default contains %d. so, this check is necessary.
we could change the default to not include %d but that may break existing setups?

Copy link
Member

Choose a reason for hiding this comment

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

Or change the default value to

Value: fmt.Sprintf("gitea-dump-%d.zip", time.Now().Unix())

Copy link
Member

Choose a reason for hiding this comment

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

some pseudo code could be:

filename := fmt.Sprintf("gitea-dump-%d.zip", time.Now().Unix())
if ctx.String("file") {
    filename = ctx.String("file")
}

and don't do fmt.Sprintf on what is passed in via CLI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Value: fmt.Sprintf("gitea-dump-%d.zip", time.Now().Unix())

damn. that's brilliant. i'll do that. thank you, sir!

Copy link
Member

Choose a reason for hiding this comment

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

Either that or what @techknowlogick said. Whichever seems cleaner in the code. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented @jolheiser's approach.

Copy link
Member

Choose a reason for hiding this comment

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

🎉
Thanks again for the PR!

@techknowlogick techknowlogick added the type/enhancement An improvement of existing functionality label Apr 1, 2019
@techknowlogick techknowlogick merged commit dbba46c into go-gitea:master Apr 1, 2019
@glaszig glaszig deleted the cmd-dump-custom-filename branch April 14, 2019 00:17
@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.

7 participants