Skip to content

Conversation

@Razer6
Copy link
Member

@Razer6 Razer6 commented Jul 27, 2014

What does this MR do?

This PR adds validation for invalid input for branch and tag names. Validating a name is done using the git subcommand git check-ref-format. If an error occurs, an error message is shown indicating a wrong input.

This behavior is also added to the API. If an error occurs a 405 error responded.

Why was this MR needed?

Currently when entering an invalid name for branch or invalid reference name GitLab fails silently or raises a 500 error. This behavior also applies if the branch already exists.

What are the relevant issue numbers / Feature requests?

This PR fixes:

TODO

  • Add support for tag validation

Many thanks to @jvanbaarsen for his help for a ruby beginner.

@randx Creating a new branch within the spinach test doesn't work. The current testcases passes because the branch already exists in the test repository. @jvanbaarsen also looked at this problem, but doesn't know what's going on here.

Something similar happens on specs. Since there is now an explicit return value when creating a new branch, this fails and returns an error. Have you any thoughts why creating a branch doesn't work within the testsuite?

/cc @cirosantilli

@TeatroIO
Copy link

I've prepared a stage. Click to open.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jvanbaarsen Should I fix them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah please do so

Copy link
Member Author

Choose a reason for hiding this comment

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

Your wish is my command

@jvanbaarsen
Copy link
Contributor

@Razer6 Thanks for the nice MR format 🍰

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you maybe also create a spec for this class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@Razer6 Razer6 changed the title Validate branch-names and references in WebUI, API Validate branch/tag-names and references WebUI, API Jul 27, 2014
@Razer6
Copy link
Member Author

Razer6 commented Jul 27, 2014

@jvanbaarsen I added a spec for the validator and also for validation for the tags page. One question: There are 2 syntax between spinach features and steps.

    1. Both use Given/And/When Syntax
    1. Features use Given/And/When, Steps use step syntax

What to prefer?

@jvanbaarsen
Copy link
Contributor

@Razer6 I dont think I understand?

@Razer6
Copy link
Member Author

Razer6 commented Jul 27, 2014

Compare https://github.com/gitlabhq/gitlabhq/blob/master/features/steps/project/browse_branches.rb with https://github.com/gitlabhq/gitlabhq/blob/master/features/steps/project/browse_commits.rb

The first one uses step:

step 'I click link "All"' do
    click_link "All"
  end

the second uses Given

Given 'I click atom feed link' do
    click_link "Feed"
  end

@jvanbaarsen
Copy link
Contributor

@Razer6 I think step is better, since it does not limit you to a single way (Given, When etc..) But not sure what @randx prefers. For now go with the step

@Razer6
Copy link
Member Author

Razer6 commented Jul 28, 2014

The tests fail because adding a branch/tag fails within the testsuite. I think the problem is, that there is no gitlab-shell. Am I right here?

@cirosantilli
Copy link
Contributor

I also need to edit files / create branches on the tests for #7266 . This is a blocking issue for the feature.

The following existing test is a false pass:

Scenario: I create a branch

Because the "newly" created branch deploy_keys already exists in the seed repo:

step 'I submit new branch form' do

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a falsely passing test because the deploy_keys branch already exists in the seed. Proposed solution at: step 'I submit new branch form' and step 'I submit new branch form' use a branch name that does not exist like not_yet_a_branch instead of deploy_keys.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I know I already pinged @randx 5a1429a

@Razer6
Copy link
Member Author

Razer6 commented Aug 1, 2014

Additional, I needed to add whitespace stripping to find valid references. @randx Have you an idea how to implemement the tests here?

@Razer6
Copy link
Member Author

Razer6 commented Aug 9, 2014

With the refactored tests from @randx all testcases pass locally. @jvanbaarsen Can you review?

@cirosantilli
Copy link
Contributor

For future reference, the commit that fixes the tests is: e09ef2f, and now no more huge .gz seed: a small test repo is cloned from https://gitlab.com/gitlab-org/gitlab-test once when the tests are run. WAY TO GO!!

@Razer6
Copy link
Member Author

Razer6 commented Aug 15, 2014

@randx rebases, all tests pass locally

@jvanbaarsen
Copy link
Contributor

@Razer6 Can you please squash the commits?

@Razer6
Copy link
Member Author

Razer6 commented Aug 15, 2014

@jvanbaarsen I squashed the commits, but I still separated between branches and tags.

@jvanbaarsen
Copy link
Contributor

@Razer6 Ok, thanks!
@randx Ready to merge!

@dosire
Copy link
Member

dosire commented Sep 1, 2014

@randx ping

@Razer6 Razer6 force-pushed the git_ref_validation branch from 886d275 to 3921139 Compare September 3, 2014 11:08
@Razer6
Copy link
Member Author

Razer6 commented Sep 3, 2014

@randx Rebased!

@dzaporozhets
Copy link
Contributor

looks good

dzaporozhets added a commit that referenced this pull request Sep 4, 2014
Validate branch/tag-names and references WebUI, API
@dzaporozhets dzaporozhets merged commit 9bb1d8f into gitlabhq:master Sep 4, 2014
@Razer6 Razer6 deleted the git_ref_validation branch September 4, 2014 12:55
@dosire
Copy link
Member

dosire commented Sep 4, 2014

Awesome functionality, thanks and congratulations @Razer6 !

@jvanbaarsen Thanks Marshal!

@Razer6
Copy link
Member Author

Razer6 commented Sep 4, 2014

Yeah, the first bigger PR i did ❤️

@dosire
Copy link
Member

dosire commented Sep 4, 2014

Yes, consider updating http://www.rschilling.net/ to include Ruby :-)

@Razer6
Copy link
Member Author

Razer6 commented Sep 4, 2014

@dosire Good catch 👍

@dosire
Copy link
Member

dosire commented Sep 4, 2014

@Razer6 Also kidding a bit, but awesome stuff! Secretly hoping it will keep raining in Austria for 2 years (except for January when I visit of course).

@dosire
Copy link
Member

dosire commented Sep 4, 2014

@Razer6 Again kidding of course 💟

@Razer6
Copy link
Member Author

Razer6 commented Sep 4, 2014

😏 When are coming to Austria? Skiing in the winter? I have a call with @randx tomorrow to discuss some things, expect awesomeness!

@dosire
Copy link
Member

dosire commented Sep 4, 2014

@Razer6 Skiing, no that is for your people (kidding, I love that and snowboarding). But we crazy Dutch people prefer speed skating http://www.weissensee.nl/ I'll sign up for 200km at the end of January.

@Razer6
Copy link
Member Author

Razer6 commented Sep 4, 2014

@dosire There is a Dutch hompage for an Austrian lake 😲 😲? You are really crazy 😄

@dosire
Copy link
Member

dosire commented Sep 4, 2014

@Razer6 We are, you should see my finish video of last year, I was pretty happy to conclude my first year of iceskating http://live.ultimate.dk/video/front/?eventid=220108&language=nl&pid=7297&timefield=Lap&videoid=18&timeday=65173000&timerace=11:01:06&timingpoint=175%20km&timeid=14

@Razer6
Copy link
Member Author

Razer6 commented Sep 4, 2014

OMG 😱 175km in one run? After 175km you can really be happy. Congrats 👍

@dosire
Copy link
Member

dosire commented Sep 4, 2014

@Razer6 Thanks!

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

Development

Successfully merging this pull request may close these issues.

6 participants