Save tags and branches properly #320
Conversation
| end | ||
|
|
||
| add_index :branches, :repository_id | ||
| add_index :branches, [:name, :repository_id], unique: true |
There was a problem hiding this comment.
Are both of these indices necessary, or is only the last one needed? I've seen Postgres use only one or some of the columns in a multi-column index before.
There was a problem hiding this comment.
PG in general can use multi column indexes for single column queries, but it will be less performant - the first field in multi column index will be the fastest to check. Maybe it's a premature optimization, but queries by name and by repository_id will be done every time new build is added, so I assume that having 2 indexes will help and rather not hurt.
|
I continued work on this today (specifically on figuring out and starting next steps) and one thing really bothers me. I linked builds to branches and tags, but maybe I should do such a connection with commit? My initial thoughts on this were: the thing that we expose in the UI are builds, commits are mostly "attached" to the build. Even if we have some kind of redundancy for commits, it's totally ok. On the other hand, it would be nice to have a model as close to git as possible with exactly one unique commit with any given sha, which is linked to tags and branches. Then build can be linked to multiple branches, but it also can be linked to only one branch (if a given project decides to do build for each push). This will have less redundancy commit-wise, but on the other hand data model will be a bit more complicated (we will need to keep info on both builds <-> branches/tags and commits <-> branches/tags relationships). What do you guys think about this? |
| @@ -8,6 +8,12 @@ class Commit < Travis::Model | |||
|
|
|||
| validates :commit, :branch, :committed_at, :presence => true | |||
|
|
|||
| def tag_name | |||
| if ref | |||
| ref.scan(%r{refs/tags/(.*?)$}).flatten.first | |||
There was a problem hiding this comment.
What does refs typically look like?
I don't think ? is necessary in this Regexp.
There was a problem hiding this comment.
You're right, ? won't change how it works. refs look like: refs/tags/v1.0.0 or refs/tags/dev/foobar etc.
There was a problem hiding this comment.
I don't know if it's more performant or the performance matters at all, but personally, I find
%r{refs/tags/(.*)}.match(ref)[1]easier to read. Also, we are assuming there is a match. Is that guaranteed?
There was a problem hiding this comment.
Is that guaranteed?
Nope, it can also be refs/heads/foobar if it's a branch.
There was a problem hiding this comment.
Ah, Regexp#match doesn't work here, since there might not be a match. On the other hand, String#scan does, since it returns [] if there is no match.
|
@drogus Let's rebase this bad boy, so that we can assess it better! |
Sure, I plan to get it updated tomorrow. |
| before { commit.ref = 'refs/tags/fooabr' } | ||
|
|
||
| it 'returns tag name' do | ||
| commit.tag_name.should be_nil |
There was a problem hiding this comment.
This is obviously wrong.
commit.tag_name.should == 'foobar'There was a problem hiding this comment.
Yeah, this test fails, I forgot to fix it before, doing it now.
|
After thinking about relationships here a bit more:
|
|
@drogus what’s the status here, can this be merged and used? |
This commits adds proper saving of tag and branch names after build creation. The information is not used anywhere for now and thus commit.branch and build.branch is still populated for backwards compatibility.
Without caller, deprecation warning will not properly report the place where deprecation happend.
This is a first part of tags and branches refactoring. It will start collecting proper tags/branch info in a separate tables.
The next step will be to use these tables in places where we need branches info.
A big question here is: can we just switch to the new way of saving info about branches without porting info from archive builds? I'm a bit torn on this: if we need to go through all the builds and populate branches/tags tables, it will take quite a lot of time. On the other hand if we don't do it, branches list for most repos will be empty after the switch.