[WIP] Use last created build to determine status image #319
Conversation
|
@henrikhodne: Do you know why the database query I'm using doesn't seem to be working? |
|
@henrikhodne: Do you know what's wrong with the query I'm using? |
There was a problem hiding this comment.
I'm not sure if this is correct. This means that the status image could have "pending" (created/queued/running) as a status instead of just passed/failed/errored.
There was a problem hiding this comment.
@henrikhodne: I think you're right. Do you know how I would get the last created (not the last finished) build?
There was a problem hiding this comment.
Build#last_build_on sorts by ID, so I think that is the current behaviour?
There was a problem hiding this comment.
@henrikhodne: I don't think so. Take a look at the builds for this repo. The most recent build is failing, but I restarted an earlier build that's passing. The build status image shows passing, even though the last created build is failing.
There was a problem hiding this comment.
That's probably because the cache is being overwritten, the DB query still returns the correct data:
>> repo = Repository.where(owner_name: "Aaron1011", name: "travis-npm-deploy").first
=> #<Repository id: 1356021>
>> build = repo.last_build_on("master")
=> #<Build id: 16326767>
>> build.state.to_sym
=> :failed
There was a problem hiding this comment.
@henrikhodne: Okay, then. So it looks like the status image behavior won't need to be changed.
There was a problem hiding this comment.
No, I think this change needs to be made in the StatesCache addon.
There was a problem hiding this comment.
@henrikhodne: I've changed it. I'll write a test soon.
lib/travis/model/repository.rb
Outdated
|
@Aaron1011 This needs to use the updated method name from #318, then I think this can be merged. |
|
@henrikhodne: Okay, I've changed it. |
|
@henrikhodne: Can you restart the |
|
Merged as 1b427c8. |
There was a problem hiding this comment.
@Aaron1011 @henrikhodne just to confirm, does this change mean that the states cache is only updated when the build is for the default branch? (eg. master)
There was a problem hiding this comment.
That is the purpose of the PR, yes.
There was a problem hiding this comment.
The states cache stores per branch info in Redis, so this means we are not storing all info in Redis anymore, meaning requests for other branches (.png?branch=foo) would have to be checked with the DB. Is this really what we want?
There was a problem hiding this comment.
Oh right, I'm sorry, I forgot that's what the state cache was doing. I agree that this change is wrong, then. Feel free to revert, otherwise I can do it tomorrow.
There was a problem hiding this comment.
If it's alright I'll leave it for you to look into tomorrow. (I need to disappear shortly)
|
I reverted this commit. See c868d4d. |
|
@henrikhodne @joshk: I think the correct behavior is to only update the cache if the build is the most recent on the given branch, instead of on the default branch. Does that sound right? |
|
Ping! @Aaron1011's last comment sounds like to me after a cursory glance. Can anyone else confirm? @henrikhodne @joshk @BanzaiMan @svenfuchs |
|
Sorry, I'm going to close this for now as we have some big changes in the works which will fix this on the UI level first, then in the back end code. |
|
@joshk Any publicly accessible reference for those incoming changes that I can track? |
|
Not at the moment sorry. If you would like to apart of some UI testing send an email to support. (the page I am user testing will give you some insight into what is coming in Feb) |
|
My head is spinning with all this activity on a PR I made over a year ago :) |
5 participants
Don't merge this yet.