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 generic URL option for status png to make forking readmes on github display correct repo #779

Closed
DavidJFelix opened this Issue Nov 28, 2012 · 24 comments

Comments

Projects
None yet
8 participants
@DavidJFelix
Copy link

DavidJFelix commented Nov 28, 2012

Feature request:

The current status icon is great for many things, but has its own issues within github.

Say I have a project and the readme has a reference to the status icon, http://travis-ci.org/DavidJFelix/example.png . John Doe forks my project and does a bunch of work, but his readme points to my status icon, regardless of whether or not he has travis-ci setup. This can mean inaccurate information is displayed. This means that perhaps John will need to ignore certain parts when pushing/pulling and make use of filter-branch more frequently.

Say for another example that I'm a travis-ci zealot and I'm telling a friend how helpful travis-ci can be for projects! I see his project sitting out on github and I think "I know! I'll just patch travis-ci support for him, and all he has to do is turn it on in the admin panel". If I send him a pull request, I have to either set up a hard address for him to pull (this can be further complicated if my friend is organization, and the person who turns on travis could be any admin member of the group).

My proposal is to expand the current controller that handles status images to accept a static route (perhaps travis-ci.org/status_img.png) and check the referer URL. If the URL doesn't exist or can't be linked to a travis user/repo - it reports unknown status. If it parses as a github repository and can be found as a linked repository, display the icon! That way forks can immediately reflect build status.

This could also be expanded to be more reliable. The current solution proposed could be unreliable if the client doesn't report referer url or if an outside site strips the readme for internal documents. In these cases, perhaps the github hooks api could be used to make travis aware of forks that exist/the network graph. This is only really necessary if the referer url proves to be too reliable.

I tried to get help implementing this myself, but I just can't be bothered to learn rails 3 just to implement a 3-10 line pull request - I know it's this simple, but I haven't used rails in years so I can't figure out how. I'll donate $35USD to travis in the name of any person who helps me implement this - regardless of whether the pull request is accepted or not.

@drogus

This comment has been minimized.

Copy link
Member

drogus commented Nov 28, 2012

Displaying "unknown" status if url does not match certain URL, will be petty limiting - people can add status images not only in the README on github (and even if only there, sometimes README is displayed also in other locations, like docs). That's why I'm -1 on this specific solution.

What we could do is to display image for a main repository by default, display image for fork if url matches one of the github's fork urls and allow to whitelist other locations for forks. However, I'm not sure how realistic is merging such thing at the moment - please note that after implementing this code, we need to support and scale it for thousands of repos. The other problem is that putting such whitelist in .travis.yml is not an options and we don't have any kind of per repo settings available to the users.

That said, I certainly would like to further discuss, please just not be so fast on implementation before we can discuss the specifics.

@DavidJFelix

This comment has been minimized.

Copy link
Author

DavidJFelix commented Nov 28, 2012

The idea is not to replace the current /[user]/[repo].png for most applications, but to add an alternative URL designed for the volatility of a github README's location. If, from the above examples John Doe forks my repository, technically speaking, the build status of his repository is not known until he sets up travis for the repo. You know that mine builds and happily advertise that on his page, but when he makes a thousand broken changes the icon will still say "passing" when in fact nobody has any idea. In addition, if somebody pulls down a static version and puts it in their repository and you view their readme there... the build status is not known - the build they took down could have been ages ago, but the icon will happily display "passing" because now the software passes. The point is that travis-ci integrates with github and that github urls are very reliable when it comes to finding repositories on travis's side - if it's not github and the user on github doesn't have a project by the name that's asking then you can't possibly know the build status - because it's not in travis.

I'm not looking for anything people have to change in the .travis.yaml or configure. All they would have to do is change it in their readme, and if it turned out that too many people block referer URLs from being sent in HTTP headers and their image displays unknown, they could switch back. As long as the current system stays in-tact it literally could be one of the lowest risk changes -- but, that's beside the point. While I'd like to see the functionality, all I really want is for somebody to tell me how (in travis's code) I can do what I'm thinking. If this implementation isn't right, the Pull Request can get rejected. It's not that I care about my code making it into travis, I'm just utterly frusterated with learning the entirety of Rails 3 just so I can know how exactly to write a route for this and how/where to use Rails' built in HTTP-referer. So, in my mind I'd be happy and satisfied with just information on how to fix this myself, which boils down to the following questions:

  • In /config/routes.rb, how do I take the information from line 44 and make a new route that points to a static page? What additional information needs to be changed for this route to work? Do I need to use a new controller or can I just modify repositories#show?
  • In /app/controllers/vi/respositories_controler.rb how do I add a controller function or modify show so that it gets data from rails' referer and fills in the redirect?

So let me explain again. I know the fix in my mind is literally less than a dozen LOC changes... I can see it above 1 route, 1 small controller function that uses 2-3 builtin or pre-existing functions. Whether it is valid or not can be left to discussion on a pull request, or here for all I care, but if somebody helps me write those dozen LOC and feed my insatiable hunger to know the answer (without having to be a Rails 3 expert - I don't need that) I'll donate to the project. A pull request isn't rushing implementation. Anyone else can counter it with their own suggested changes.

@drogus

This comment has been minimized.

Copy link
Member

drogus commented Nov 28, 2012

@DavidJFelix I could tell you where to implement it, but we would like to retire rails app soon anyway and then status images would be probably moved elsewhere, which I think it's not decided yet. If you implement it now, we will either have to port it later or wait for someone else to do it. I'm not saying that we don't want this feature at all and I do realize that we can reject the pull request if we don't like it, but this is definitely not high priority thing at the moment.

@DavidJFelix

This comment has been minimized.

Copy link
Author

DavidJFelix commented Nov 28, 2012

@drogus would you tell me where to implement it? :)

@sarahhodne

This comment has been minimized.

Copy link
Contributor

sarahhodne commented Nov 29, 2012

@DavidJFelix The status image responder can be found in lib/responders/result_image.rb. The controller is in app/controllers/v1/repositories_controller.rb.

@bjornstar

This comment has been minimized.

Copy link

bjornstar commented Jan 16, 2013

I would really love to have this feature. I'm not enjoying re-writing my readme files every time I do a pull request to a different fork. I think the referrer approach sounds great.

@defunctzombie

This comment has been minimized.

Copy link

defunctzombie commented Jan 25, 2014

Was the rails app ever retired? This issue has seen no activity but would be quite useful. The readme badges are misleading in forks and branches otherwise.

@drogus

This comment has been minimized.

Copy link
Member

drogus commented Jan 27, 2014

@defunctzombie yes. At the moment API is handled by sinatra application travis-api. Images are handled in image responder, which basically calls Repository::StatusImage class.

@defunctzombie

This comment has been minimized.

Copy link

defunctzombie commented Jan 27, 2014

@drogus which file in the sinatra app is the route handler for the images routes?

@roidrage

This comment has been minimized.

Copy link
Contributor

roidrage commented Jan 29, 2014

After this: https://github.com/blog/1766-proxying-user-images I don't think this is doable at this point.

@defunctzombie

This comment has been minimized.

Copy link

defunctzombie commented Jan 29, 2014

It is doable. You just send the "no-cache" headers.

@roidrage

This comment has been minimized.

Copy link
Contributor

roidrage commented Jan 29, 2014

Yup, but the proxy scraps user data from the request.

Without a referrer, how do you want to determine the repository?

@defunctzombie

This comment has been minimized.

Copy link

defunctzombie commented Jan 29, 2014

Does it actually scrape the origin header? All you need to know is the page making the request. Hopefully their proxy is not trying to be too clever. My guess is that it just removes cookies and other sensitive items.

@sarahhodne

This comment has been minimized.

Copy link
Contributor

sarahhodne commented Jan 29, 2014

The only headers that are transferred seem to be User-Agent, Accept and Accept-Encoding.

@defunctzombie

This comment has been minimized.

Copy link

defunctzombie commented Jan 29, 2014

(╯°□°)╯︵ ┻━┻

@DavidJFelix

This comment has been minimized.

Copy link
Author

DavidJFelix commented Jan 29, 2014

┬─┬ノ( º _ ºノ) Would it be hard to also transfer referrer? Is there a downside to that?

@sarahhodne

This comment has been minimized.

Copy link
Contributor

sarahhodne commented Jan 29, 2014

That would be up to GitHub, I think. However, I think the referrer is considered sensitive as well, which is part of the reason for this proxying being done, I believe.

@DavidJFelix

This comment has been minimized.

Copy link
Author

DavidJFelix commented Jan 29, 2014

I'm trying to figure out how referrer might be sensitive and can't put my finger on it. Github URLs are pretty generic -- I guess they might have information about private repos, but that would only be name data, which I'm fairly sure is exposed through user activity pages as well. I wonder if they could be persuaded to accept a patch to add referrer.

@roidrage

This comment has been minimized.

Copy link
Contributor

roidrage commented Feb 19, 2014

With GitHub now filtering this data, I'm closing this issue until we find a possible means to implement this.

@roidrage roidrage closed this Feb 19, 2014

@ilanbiala

This comment has been minimized.

Copy link

ilanbiala commented Jul 22, 2015

Has this been implemented in some way?

@DavidJFelix

This comment has been minimized.

Copy link
Author

DavidJFelix commented Jul 22, 2015

@ilanbiala nope. Github's "camo" proxy prevents referrer from being passed and therefore the server from detecting where the request is originating.

@sbelsky

This comment has been minimized.

Copy link

sbelsky commented Feb 5, 2016

And so what? Is is impossible to make directly agreement between travisci and github project teams about communication protocol?

@DavidJFelix

This comment has been minimized.

Copy link
Author

DavidJFelix commented Feb 5, 2016

Impossible? No. Infeasible? Yes. Github camo intentionally obfuscates this data to prevent external tracking like what I had initially suggested. While a whitelist position is possible, I'd assume it's unlikely as there are no currently whitelisted services.

@defunctzombie

This comment has been minimized.

Copy link

defunctzombie commented Feb 5, 2016

While camo itself obfuscates the referrer one possible solution is for
github to just add custom headers with the repo and branch info. Since they
make the request knowing this information they can pass it along. This
would require changes to github tho and not camo.

On Friday, February 5, 2016, David J. Felix notifications@github.com
wrote:

Impossible? No. Infeasible? Yes. Github camo intentionally obfuscates this
data to prevent external tracking like what I had initially suggested.
While a whitelist position is possible, I'd assume it's unlikely as there
are no currently whitelisted services.


Reply to this email directly or view it on GitHub
#779 (comment)
.

niklas88 added a commit to ad-freiburg/QLever that referenced this issue Mar 13, 2018

Change CI badge to point to ad-freiburg/master
Sadly there is currently no way to automatically detect the right branch see also travis-ci/travis-ci#779
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.