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

Should SSH access be checked in the master repo? #118

Closed
squarefrog opened this issue Apr 25, 2014 · 13 comments · Fixed by #153
Closed

Should SSH access be checked in the master repo? #118

squarefrog opened this issue Apr 25, 2014 · 13 comments · Fixed by #153

Comments

@squarefrog
Copy link
Contributor

We had an issue pop up in the specs repo CocoaPods/Specs#10877 where a contributor submitted a spec using git@github...

However, specs should be using https. I notice the local linter passes this fine, which I can understand for private repos, but I wonder if this behaviour should not pass if contributing to the master repo?

There's quite a discussion in that pull request so I thought it might be more appropriate to raise an issue here instead.

@orta
Copy link
Member

orta commented Apr 25, 2014

odd I thought we were linking for making sure all github links used https.

@neonichu
Copy link
Member

Nah, the goal is to not fail on private spec repos, see #83. The GitHub check only verifies that you don't use http.

@squarefrog
Copy link
Contributor Author

https://github.com/CocoaPods/Core/blob/master/spec/specification/linter_spec.rb#L266-L276

I don't suppose there is a method to check if the repo is the master? Maybe a kludgey hack would be to check if the lint is running on Travis LOCALLY in the master repo, but that definitely makes me feel uncormfortable enough that it is not the correct solution

@neonichu
Copy link
Member

It is already failing on Travis, though, with a somewhat understandable error message.

Personally, I can only see the goal to have people realise this before making a PR, for that the lint commands would need a switch to specify they are for the master repo or one to specify that they are for a private one.

@squarefrog
Copy link
Contributor Author

I'm sorry yes that was a mistake on my part - I believe the issue was I linted the file locally and it passed which confused me when providing support in Specs.

It's definitely something I can manually look out for now, but it could be an opportunity to clear things up when locally linting.

@fabiopelosin
Copy link
Member

@neonichu when trunk will be introduced that full check performed by Travis will be removed (because waiting for the pull-request made Trunk too much complex... if I'm correct... @alloy ?). So @squarefrog makes sense in my opinion.

We could pass to the Validator whether we are submitting to the master repo and in that case run additional checks.

@neonichu
Copy link
Member

Ah, I see.

I would suggest assuming the master repo by default and passing something to the Validator if some other spec repo is used. That way it would do the right thing for inexperienced users.

@squarefrog
Copy link
Contributor Author

👍

@alloy
Copy link
Member

alloy commented Apr 30, 2014

because waiting for the pull-request made Trunk too much complex... if I'm correct...

@irrationalfab Aye, it adds complexity while not educating spec authors, as some use it as a way to get their specs in order instead of trying in a real app etc.

@alloy
Copy link
Member

alloy commented Apr 30, 2014

I agree with @neonichu.

At some point it might be nice if you could have a default options config inside a spec repo. That way private spec repos could indicate what type of linting they want etc.

@fabiopelosin
Copy link
Member

At some point it might be nice if you could have a default options config inside a spec repo. That way private spec repos could indicate what type of linting they want etc.

Is there any benefit in maintaining this?

@alloy
Copy link
Member

alloy commented Apr 30, 2014

The idea is that you then no longer have to specify certain options for certain repos. However, this only makes sense if we really really really need options at all.

@fabiopelosin
Copy link
Member

I think that having a higher set of checks for just the master repo is simpler and sufficient for the moment.

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 a pull request may close this issue.

5 participants