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

NOT MERGED: Add bucket storage support #9567

Merged
merged 0 commits into from
Aug 17, 2020
Merged

Conversation

cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Dec 31, 2019

THIS PR WAS NOT MERGED

REPLACED BY #11387

PLEASE NOTE This PR was not merged and has been labelled merged in error due to force-pushing master before closing the PR

It was replaced by #11387

For future reference the commits that were on this branch are available at https://github.com/zeripath/gitea/tree/pr-9567

Many thanks to @cuonglm for their hard work on this PR - even though the work was not merged it was helpful and appreciated.


This is a follow up PR for #7795, cherry-pick those commits and add:

  • Abstract filesystem layer for avatars/attachments/lfs objects, using afero
  • Fix some issue with test, error checking
  • Add tests for bucket storage using minio (for s3 compatibility) and fake-gcs-server (for GCS)
  • Add storage-migrate command to migrate from local to cloud storage and vice versa.

Running:

gitea storage-migrate --bucket='s3://gitea?endpoint=localhost:9000&disableSSL=true&s3ForcePathStyle=true&region=us-east-1'

will migrate from local to cloud storage, and:

gitea storage-migrate --local --bucket='s3://gitea?endpoint=localhost:9000&disableSSL=true&s3ForcePathStyle=true&region=us-east-1'

will migrate back from cloud to local.

@cuonglm cuonglm force-pushed the master branch 6 times, most recently from c13ed6b to 0b24589 Compare December 31, 2019 17:38
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 1, 2020
@lunny lunny added this to the 1.12.0 milestone Jan 1, 2020
.drone.yml Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 1, 2020
Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

I think all temp dir should be local file system.

cmd/dump.go Outdated Show resolved Hide resolved
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

Thank you for this PR 😄 very excited for it.

I've gone through and noticed a few items with import structures, and noted a few of them (there are others that I haven't noted, but should still also be resolved)

models/attachment.go Outdated Show resolved Hide resolved
modules/storage/storage.go Outdated Show resolved Hide resolved
modules/storage/storage.go Outdated Show resolved Hide resolved
cmd/storage.go Outdated Show resolved Hide resolved
integrations/git_test.go Outdated Show resolved Hide resolved
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Mar 28, 2020
@stale stale bot removed the issue/stale label Mar 28, 2020
@6543
Copy link
Member

6543 commented Apr 20, 2020

please resolve conflicts

@cuonglm cuonglm force-pushed the master branch 4 times, most recently from d6d1f15 to eb6fac9 Compare April 21, 2020 06:43
@i0x71
Copy link

i0x71 commented Apr 27, 2020

Is there any chance of storing all of the git data on the s3 backend ?

Thanks

@cuonglm
Copy link
Contributor Author

cuonglm commented Apr 28, 2020

Is there any chance of storing all of the git data on the s3 backend ?

Thanks

I think there's likely no plan to support that feature.

@lunny lunny mentioned this pull request May 12, 2020
3 tasks
@lafriks lafriks modified the milestones: 1.12.0, 1.13.0 May 16, 2020
@techknowlogick techknowlogick merged commit 78cbd0c into go-gitea:master Aug 17, 2020
@zeripath
Copy link
Contributor

I've saved the old contents of this PR as the pr-9567 branch in my fork zeripath.

Was this force push intentional?

@lunny
Copy link
Member

lunny commented Aug 18, 2020

Why this PR' state is merged but changed files is 0? Could you review #11387 ?

@lunny lunny removed this from the 1.13.0 milestone Aug 18, 2020
@zeripath
Copy link
Contributor

@lunny the pr branch has been force pushed to master therefore there are no longer any differences between it and master, and GitHub has declared it merged. (The same thing happens in Gitea - where incidentally people complained it was a bug.)

Has #11387 replaced this PR?

@lunny
Copy link
Member

lunny commented Aug 18, 2020

@zeripath Oh. I see.

Yes, #11387 just merged and replaced this PR. I will send planned PRs on that PR.

@patcito
Copy link
Contributor

patcito commented Aug 18, 2020

@zeripath Sorry, not intentional. We can repush the old commits if you want but it looks like this PR was rejected anyway to be replaced with #11387. Is this the case @lunny ?

@zeripath zeripath added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Aug 18, 2020
@zeripath
Copy link
Contributor

@patcito AFAIU that will not revert the mislabelling of the PR being merged. I suspect it is not a huge problem - the PR was not on a milestone, it now has the skip-changelog label, and its functionality has been replaced. I've changed the top comment to reflect the situation. I could change the PR title but it might be too much.

Feel free to try to repush the old changes but I suspect that won't change the status of the PR.

@zeripath zeripath changed the title Add bucket storage support NOT MERGED: Add bucket storage support Aug 18, 2020
@patcito
Copy link
Contributor

patcito commented Aug 18, 2020

@zeripath @lunny this PR used go-cloud which supports Google cloud storage and azure. Would you accept contributions that add support to Google and azure using go cloud?

@lunny
Copy link
Member

lunny commented Aug 18, 2020

@patcito On my PR, I defined a minial interafce and I think it's easy to implement. We have local and minio two implementations and new implementation are welcome!

@a1012112796
Copy link
Member

@lunny the pr branch has been force pushed to master therefore there are no longer any differences between it and master, and GitHub has declared it merged. (The same thing happens in Gitea - where incidentally people complained it was a bug.)

Has #11387 replaced this PR?

@lunny @lafriks I have checked this bug carefully, I found the reason is that the results of fast-forward style merge and just make head branch same with base branch by force-push are the same, It's hard to distinguish between them by "git" cmd. It's really a hard thing... :(

ref:

// Get the commit from BaseBranch where the pull request got merged
mergeCommit, err := git.NewCommand("rev-list", "--ancestry-path", "--merges", "--reverse", cmd).
RunInDirWithEnv("", []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()})
if err != nil {
return nil, fmt.Errorf("git rev-list --ancestry-path --merges --reverse: %v", err)
} else if len(mergeCommit) < 40 {
// PR was fast-forwarded, so just use last commit of PR
mergeCommit = commitID[:40]
}

@zeripath
Copy link
Contributor

yes @a1012112796 that is what I was meaning, and as this PR demonstrates Gitea replicates GH's behaviour exactly.

In Gitea we could do something different whereby if a force-push to master occurs we close the PR instead but at present it appears we replicate GH exactly.

@a1012112796
Copy link
Member

a1012112796 commented Aug 19, 2020

@lunny @lafriks maybe we can use same way with gitlab: not check manually merge and allow create pr without any commits, just block it to be merge is enough.

image

@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
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.