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

When using API CreateRelease set created_unix to the tag commit time #11218

Merged
merged 6 commits into from Apr 30, 2020

Conversation

jasder
Copy link
Contributor

@jasder jasder commented Apr 26, 2020

When creating a release with api, the release time of the version is wrong(is actually the user's update time)

Ex:
image

@6543
Copy link
Member

6543 commented Apr 26, 2020

pleace "update" this branch

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 26, 2020
@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 Apr 27, 2020
Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

This looks good to me, but the current behavior looks kind of fishy... almost intended. Can this be considered a matter of preference? This PR will not affect just the API.

@6543
Copy link
Member

6543 commented Apr 28, 2020

same thought - but think this behaviour more intuetive - to controle it via an option would probably be the best ...

@6543
Copy link
Member

6543 commented Apr 28, 2020

@jasder change this pull so it can be controled via option

@lunny lunny added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Apr 28, 2020
@lunny
Copy link
Member

lunny commented Apr 28, 2020

A release is based on a tag, but should release created time be the same as the tag time? Since tag time has been saved on git, so it's unnecessary to save it again in the database.

@jasder
Copy link
Contributor Author

jasder commented Apr 29, 2020

@lunny But release's the return created time is based on release, and rend directly from the database.

Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

OK, I think this makes sense.

@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 Apr 30, 2020
@zeripath zeripath changed the title FIX [API] create release's created_unix with incorrect timestamp is bug When using API CreateRelease set created_unix to the tag commit time Apr 30, 2020
@lafriks lafriks added this to the 1.12.0 milestone Apr 30, 2020
@codecov-io
Copy link

Codecov Report

Merging #11218 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11218      +/-   ##
==========================================
+ Coverage   43.29%   43.30%   +0.01%     
==========================================
  Files         605      605              
  Lines       86246    86246              
==========================================
+ Hits        37340    37351      +11     
+ Misses      44305    44301       -4     
+ Partials     4601     4594       -7     
Impacted Files Coverage Δ
services/release/release.go 27.04% <100.00%> (ø)
services/pull/temp_repo.go 31.62% <0.00%> (-2.57%) ⬇️
modules/queue/unique_queue_disk_channel.go 53.84% <0.00%> (-1.93%) ⬇️
services/pull/check.go 55.48% <0.00%> (-1.22%) ⬇️
modules/queue/workerpool.go 56.93% <0.00%> (-1.07%) ⬇️
models/issue.go 52.10% <0.00%> (+0.46%) ⬆️
modules/notification/webhook/webhook.go 40.20% <0.00%> (+0.58%) ⬆️
services/pull/pull.go 34.54% <0.00%> (+1.14%) ⬆️
services/pull/patch.go 67.83% <0.00%> (+2.09%) ⬆️
modules/process/manager.go 78.31% <0.00%> (+3.61%) ⬆️

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 d0e7361...131f06d. Read the comment docs.

@zeripath zeripath merged commit 4468b0b into go-gitea:master Apr 30, 2020
@richmahn
Copy link
Contributor

@lunny @jasder I am having problems due to this little change. Even if you update the content of a release (the message such as make a change to the changelog in the content), the created_unix time of the release gets updated again! It isn't when the tag was created any more.

@richmahn
Copy link
Contributor

So somewhat related, in that this shouldn't be set when a tag is already set (before it didn't matter since it was going by the commit date that the tag pointed to): #12341. Please make backport to 1.12 if possible.

@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. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants