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

Add a validator for git / http sources #137

Merged
merged 8 commits into from Jul 6, 2015
Merged

Conversation

orta
Copy link
Member

@orta orta commented Jun 4, 2015

This is some of the initial work around #136

I'm struggling with figuring out how to test this and whether it is safe. Assigning to @alloy to give a one over.

end

def validate_http
system("curl -IL #{ Shellwords.escape @specification.source[:http] } -f")
Copy link
Member

Choose a reason for hiding this comment

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

please, no subshells

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't think of a way to do the git lint without a subtle

Copy link
Member

Choose a reason for hiding this comment

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

system(‘curl’, ‘-IL’, @specification.source[:http], ‘-f’)

On Jun 4, 2015, at 5:44 PM, Orta notifications@github.com wrote:

In app/models/specification_wrapper.rb #137 (comment):

@@ -48,6 +49,20 @@ def validation_errors
results
end

  •  def validate_public_access
    
  •    return validate_http if @specification.source[:http]
    
  •    return validate_git if @specification.source[:git]
    
  •    true
    
  •  end
    
  •  def validate_http
    
  •    system("curl -IL #{ Shellwords.escape @specification.source[:http] } -f")
    
    I couldn't think of a way to do the git lint without a subtle


Reply to this email directly or view it on GitHub https://github.com/CocoaPods/trunk.cocoapods.org/pull/137/files#r31780251.

Copy link
Member Author

Choose a reason for hiding this comment

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

ahhh, ok

Copy link
Member

Choose a reason for hiding this comment

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

also, will want to redirect out & err to /dev/null

Copy link
Contributor

Choose a reason for hiding this comment

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

@segiddins
Copy link
Member

I can write up specs tomorrow, if you'd like.
I also want to know how @floere feels about doing even more stuff in a synchronous request.

@floere
Copy link
Member

floere commented Jun 5, 2015

@segiddins I can search my feelings ;) Not sure what problem it solves though. Help?

@segiddins
Copy link
Member

It makes sure pods in trunk are actually public ally accessible before allowing them to be pushed.

-Samuel E. Giddins

On Jun 5, 2015, at 9:04 AM, Florian R. Hanke notifications@github.com wrote:

@segiddins I can search my feelings ;) Not sure what problem it solves though. Help?


Reply to this email directly or view it on GitHub.

@floere
Copy link
Member

floere commented Jun 5, 2015

Ah thanks. Seems weird we haven't thought of this right when writing trunk.

The current code probably needs more error handling (or rather, status codes checking) and better - and more precise - info for the user so they can act appropriately.

Is it time yet for asynchronous processing of trunk pushes and reporting the running jobs on the command line?
Something like:

Checking if publicly available? ... Yes! (<repo url>)

@segiddins
Copy link
Member

The only reason I see to do that is the heroku timeout... It's ok to me if the actual push takes a bit from the users perspective.

-Samuel E. Giddins

On Jun 5, 2015, at 9:22 AM, Florian R. Hanke notifications@github.com wrote:

Ah thanks. Seems weird we haven't thought of this right when writing trunk.

The current code probably needs more error handling (or rather, status codes checking) and better - and more precise - info for the user so they can act appropriately.

Is it time yet for asynchronous processing of trunk pushes and reporting the running jobs on the command line?
Something like:

Checking if publicly available? ... Yes! ()

Reply to this email directly or view it on GitHub.

@floere
Copy link
Member

floere commented Jun 5, 2015

@segiddins We can set the Heroku timeout, should that be a problem.

@segiddins
Copy link
Member

@floere pretty sure it's bounded at 30 seconds

@floere
Copy link
Member

floere commented Jun 5, 2015

@alloy
Copy link
Member

alloy commented Jun 8, 2015

Seems weird we haven't thought of this right when writing trunk.

Trunk was really only interested in validating data it needed to be able to do its work, ensuring the rest of the podspec data is valid was upto the client.

@floere
Copy link
Member

floere commented Jun 8, 2015

@alloy Not saying it needs to do it – saying it's weird we haven't considered the case (at least I can't remember).

@floere
Copy link
Member

floere commented Jun 8, 2015

@segiddins @orta @alloy If the long-runny-ness of this becomes an issue, I see two solutions: background jobs/multi-request (the client periodically gets the latest state – good if we have many requests for trunk), or response streaming (updating the info the client has as we go – good if we don't have many requests for trunk). Putting this here so I don't forget.

@floere
Copy link
Member

floere commented Jun 8, 2015

@alloy Would you then prefer this verification be in the client?

@alloy
Copy link
Member

alloy commented Jun 8, 2015

Yeah I did consider it then, but left it to the client. Clearly that didn't turn out to be enough, probably because people push pods from private repos that their machines have access to at the time of validating :)

@floere
Copy link
Member

floere commented Jun 8, 2015

@alloy Ah yes… good point. I do remember that we discussed that trunk act as a guard for ok pods – and that the client only does checks to provide a responsive/comfortable UI. According to that line of thought, this code would need to be in trunk.

@alloy
Copy link
Member

alloy commented Jun 8, 2015

Yeah I think it’s fine to do this now in Trunk. I think it would just be good to keep these things to a minimum and not require an extra queue :)

@floere
Copy link
Member

floere commented Jun 8, 2015

@alloy We're both from the "do the minimum that works" school :)

@alloy
Copy link
Member

alloy commented Jun 8, 2015

@floere Yeah I know, it was a general statement ;)

@orta
Copy link
Member Author

orta commented Jun 11, 2015

Got a sense of what I think it would just be good to keep these things to a minimum and not require an extra queue means here?

What do I need to do to get this wrapped up?

@alloy
Copy link
Member

alloy commented Jun 11, 2015

I think it would be a good idea to add a timeout block around the call and log how long it took to perform, then you an fine tune it after it's in use.

I'd also urge you to not use a shell at all, but instead call open/spawn with an array of arguments. That way there can never be any illicit command execution.

@orta
Copy link
Member Author

orta commented Jul 1, 2015

Stuck the landing 10/10

OK, this PR now has everything that people have asked for 👍 - I'm still unsure how to test this, any guesses?

@alloy
Copy link
Member

alloy commented Jul 1, 2015

I’d just mock the call to Pod::HTTP.validate_url, expect the right argument, and return true/false, it’s implementation is already tested in the CP gem, and add an expectation for the call to system as well?

@orta orta force-pushed the validate_spec_source branch 3 times, most recently from acd14ee to 8ab7a4e Compare July 1, 2015 16:28
@orta
Copy link
Member Author

orta commented Jul 1, 2015

Alright, this is 👍

private

def wrap_timeout(closure)
Copy link
Member

Choose a reason for hiding this comment

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

Why not take an &blk arg so you don't have to create explicit procs?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure how to do that, and the proc worked for QIs earlier /shrug

Copy link
Member

Choose a reason for hiding this comment

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

This would be a bit nicer yeah, not a show stopper, though. @segiddins can you add it, if you feel strongly about it?

end

def validate_git
wrap_timeout proc { system('git', 'ls-remote', @specification.source[:git], 'HEAD') }
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't that be @specification.source[:tag] || @specification.source[:commit] || @specification.source[:branch] || 'HEAD' ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that it's trunk, it can only be :tag, and I'm validating it exists pod spec lint will have checked that the tag exists by this point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that it's trunk, it can only be :tag, and I'm validating the repo exists - pod spec lint will have checked that the tag exists by this point.

@segiddins
Copy link
Member

@alloy can you give this a final look?

@alloy
Copy link
Member

alloy commented Jul 5, 2015

LGTM 👍

@segiddins
Copy link
Member

I'll make my few style changes, then merge tonight. Thanks @orta :)

segiddins added a commit that referenced this pull request Jul 6, 2015
Add a validator for git / http sources
@segiddins segiddins merged commit e065632 into master Jul 6, 2015
@segiddins segiddins deleted the validate_spec_source branch July 6, 2015 01:28
@alloy
Copy link
Member

alloy commented Jul 6, 2015

Nice work, fellas 👍

@orta
Copy link
Member Author

orta commented Jul 6, 2015

Thanks sir

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants