Follow-up from "Resolve "Create X-Ray scanner uploader job""
The following discussions from !138220 (merged) should be addressed:
-
@jprovaznik started a discussion: nit: Should we add it to associations in the spec https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/spec/models/ee/project_spec.rb#L13?
-
@jprovaznik started a discussion: nit: do we need this method? We can similarly call just
StoreRepositoryXrayService.new(pipeline).execute
. It doesn't seem to provide any additional value, a downside is that we will have to maintain this aditional method whenever we need to update initialization of the instance (e.g. add another param). -
@jprovaznik started a discussion: not-blocking nit: can we put class methods at the top of the class (before initialize) to respect the structure of the class? Or alternatively does this need to be a class method? It doesn't seem to be used from outside of the class so this could be a private instance method.
-
@jprovaznik started a discussion: (+1 comment) filename
is passed as a param to the outer block, should we movelang = File.basename(filename, '.json')
to the outer block too (instead of re-setting lang for each line)? -
@jprovaznik started a discussion: (+1 comment) I'm a bit confused why we do
blob.each_line
? Does it mean that each file can have multiple lines? If yes, then we just rewrite all previous lines with the last one in the file. Is this intentional?I would expect that each file contains one report (in json format), same as the example in
ee/spec/fixtures/repository_xray/gl-repository-xray.json.gz
. Should we rather removeblob.each_line
iteration and parse the whole content of the file instead? -
@jprovaznik started a discussion: Do we need this delegation? I couldn't find its usage in this class?
-
@jprovaznik started a discussion: not-blocking nit: do we need this
allow
? If it's because usinghave_received
in tests below, you could use justexpect(::Ai::StoreRepositoryXrayWorker).to receive(:perform_async)...
in specs instead (which would be simpler). -
@jprovaznik started a discussion: nit: the name suggests that
artifact.repository_xray
returns a single record (xray repository), instead it returns a list of artifacts having the xray repo file type. Would it make sense to call it something likewith_repository_xray
? -
Need a spec to cover the StorwXrayReportService.log_event
wrapper to fix undercoverage