Skip to content

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 move lang = 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 remove blob.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 using have_received in tests below, you could use just expect(::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 like with_repository_xray?

  • Need a spec to cover the StorwXrayReportService.log_event wrapper to fix undercoverage

Edited by Allen Cook