Save artifacts sizes

What does this MR do?

Introduce ci_builds.artifacts_size as an integer, so that it's easier to access than reading from the file again.

What are the relevant issue numbers?

Closes #18869 (closed)

@godfat What happens if path is in a binary format?

@godfat I think that we more needed a artifacts_size field, with a summary size of artifacts archive, which would make it possible to quickly check how much storage artifacts consume. Is that right @ayufan or did I miss some conversation about that?

@grzesiek If the path is in binary, one of them would happen (need to verify):

• Some invalid encoding exceptions would be raised
• or the string to the path would be encoded in binary (ascii-8bit)

Oh, so what we want is the size in total, rather than each individual files?

@grzesiek

Correct.

@godfat

We explored a fact of storing JSON in database, but we decided to not to do so, because this json can be 100kB (you push artifacts with linux repo).

Oh, so what we want is the size in total, rather than each individual files?

Yes :) We need to easily show a summary of all artifacts for a project. It is hard to calculate each individual file, especially that file is uncompressed :)

AFAIK we store that information already in metadata.

Reassigned to @godfat

@ayufan Got it!

mentioned in commit 0a294698

Added 1 commit:

• 0a294698 - Just save the size in total rather than individual files

@ayufan It's now changed to an integer. Most changes were for styling though.

@godfat Nice :)

Also make sure that when artifacts are removed the artifacts_size is set to nil.

@godfat

Maybe we should add to Ci::Build model:

def artifacts_file_will_change!
   self.artifacts_size = if artifacts_file.exist? then artifacts_file.size end
end

(I'm not sure if artifacts_file_will_change! will work. Maybe it does, but we can use similar approach)

@ayufan Right, I'll also add this test to erase API. I don't feel we need to use things like artifacts_file_will_change! though. If we're using service objects or so properly, we should find a good place to do that, rather than triggering some magic. What do you think?

Added 1 commit:

• de543359 - Prefer Ci::Build#erase_artifacts!

@godfat I feel that we can sometime forget about doing that and we will left artifacts_size in incosistent state, and this column make sense only when looking at it in context of artifacts_file. Having that in the first place would imply no changes to erase_artifacts! at all :)

@grzesiek What do you think?

Added 1 commit:

• 1bc0d732 - Also remove ci_builds.artifacts_size when erased

@godfat @ayufan I think that we can use CarrierWave callbacks here which should solve this problem.

@ayufan I think that's exactly the reason why we use service objects at places. We are not doing this in a lot of places so we probably won't trouble to create one service object here, but I think this also means it's ok to just modify two places for this. (create+update and delete) ActiveRecord's callbacks are often considered harmful :P Are there any other places you might think of that we could be touching artifacts? If so, I think it's probably worth to check if we want to create a service object for it.

@grzesiek We don't seem to have access to the build instance there though?

@godfat Maybe, but we use callbacks in quite a few places.

What about:

before_save :update_artifacts_size, if: ->(build) { build.artifacts_file_changed? }

It should no longer be magic :)

@ayufan As long as it's consistent (i.e. we're using callbacks in quite a few places), I think it's ok. I'll try this tomorrow. Thanks!