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

Fix import.rake failed import if project name is also an existing namespace. #8159

Merged
merged 1 commit into from Oct 24, 2014

Conversation

4 participants
@cirosantilli
Contributor

cirosantilli commented Oct 23, 2014

also an existing namespace. E.g., when trying to import
group/root.git, that would fail if there is an user called root.

The error was twofold:

  • it would execute project.name, where project is not defined
  • it should not continue before creating the project!
@TeatroIO

This comment has been minimized.

TeatroIO commented Oct 23, 2014

I've prepared a stage. Click to open.

@cirosantilli cirosantilli force-pushed the cirosantilli:fix-rake-import-existing-group branch from 2791409 to d4e29fd Oct 23, 2014

@cirosantilli cirosantilli changed the title from Fix import.rake failed import if project name is to Fix import.rake failed import if project name is also an existing namespace. Oct 23, 2014

@Razer6

This comment has been minimized.

Member

Razer6 commented Oct 24, 2014

@jacobvosmaer Looks good!

@Razer6 Razer6 added this to the 7.5 milestone Oct 24, 2014

@jacobvosmaer

This comment has been minimized.

Member

jacobvosmaer commented Oct 24, 2014

@cirosantilli what would happen if the group (foo-group) already exists but I have added repo on disk that does not exist in GitLab yet? (foo-group/new-repo.git)

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Oct 24, 2014

@jacobvosmaer

  • before this PR:
    • name is the basename name = File.basename path (new-repo.git)
    • so if namespaces.include?(name) (should be group_name1!) does not get activated, unless new-repo is the name of an existing namespace (either foo-group or another existing one).
    • If that happens, project is undefined and raises and operation stops (existing bug)
    • otherwise, everything it works as before since there was another group = Group.find_by(path: group_name) check further on.
  • after this PR: it works as expected if I did this correctly =)
    • group = Namespace.find_by(path: group_name) will find the group
    • if group gets activated and prints "Skipping #{name} due to namespace conflict with group or user".yellow which is likely the intention of the original author of that line, and skips group createion.
    • project = Projects::CreateService.new(user, project_params).execute gets run and creates the project on DB + adds hooks like before this PR (GitLab shell calls git --git-dir . init --bare on the repo at at gitlab_repositories#add_repository but man git-init says it is OK to do that on an existing repo)
# create group namespace
if !group
if group
puts "Skipping #{name} due to namespace conflict with group or user".yellow

This comment has been minimized.

@jacobvosmaer

jacobvosmaer Oct 24, 2014

Member

Maybe we should say something like:

Namespace #{name} already exists

This comment has been minimized.

@cirosantilli

cirosantilli Oct 24, 2014

Contributor

That is better. Also, how about not saying anything? Would cluttered interface: if nothing shows, assume nothing happened.

This comment has been minimized.

@jacobvosmaer

jacobvosmaer Oct 24, 2014

Member

I am OK with skipping the message.

Fix import.rake failed import if project name is
also an existing namespace. E.g., when trying to import
group/root.git, that would fail if there is an user called root.

@cirosantilli cirosantilli force-pushed the cirosantilli:fix-rake-import-existing-group branch from d4e29fd to 706b6b5 Oct 24, 2014

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Oct 24, 2014

Updated.

@jacobvosmaer

This comment has been minimized.

Member

jacobvosmaer commented Oct 24, 2014

Thanks @cirosantilli !

jacobvosmaer added a commit that referenced this pull request Oct 24, 2014

Merge pull request #8159 from cirosantilli/fix-rake-import-existing-g…
…roup

Fix import.rake failed import if project name is also an existing namespace.

@jacobvosmaer jacobvosmaer merged commit ddc4e65 into gitlabhq:master Oct 24, 2014

1 check passed

default The build passed on Semaphore.
Details

@cirosantilli cirosantilli deleted the cirosantilli:fix-rake-import-existing-group branch Oct 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment