Correct artifact registry backend review comments

Rahul Chanilarequested to merge
602638-ar-backend-review-followups into master

What does this MR do and why?

Follow-up to review threads left on two merged MRs. Comment and test-hygiene changes only. No behavior change.

1. package_class comment was wrong about Ruby

The old comment said a format missing from the case would "resolve to whichever branch happens to be last". Ruby returns nil from a case with no matching when and no else. The else is what raises, and without it the failure would surface at ee/lib/artifact_registry/client.rb:201, where the caller does row_class.new(attributes). The comment now says that.

2. repository.rb fixture header credited the wrong safeguard

The header said drift between document and schema fails here. A reviewer noted that graphql-verify already covers that repo-wide, so the sentence was a non-reason.

That premise does not hold for EE. Gitlab::Graphql::Queries.all (lib/gitlab/graphql/queries.rb:335-337) walks app/assets/javascripts and app/graphql/queries only. It never walks ee/app/assets/javascripts, where this document lives. ee/spec/graphql/all_queries_spec.rb:74 calls the same all, and checks complexity rather than validity.

So the header now carries both reasons: the JSON import in repositories/detail/repository_detail_spec.js, which the reviewer pointed at, and the drift check, stated with the reason it holds.

3. Two smaller fixture nitpicks

  • repositories_url had a single reader, its own sibling let, so the two collapse into one.
  • current_organization was a leftover name from spec/support/shared_contexts/current_organization_context.rb, which this file no longer includes. Renamed to organization, matching the sibling fixture. The rename is safe: that context auto-includes only for type: :controller, type: :graphql, or with_current_organization: true, and this spec is type: :request.
  • The let_it_be_with_refind now says why it is not a plain let_it_be. The organization strong-memoizes its client (ee/app/models/concerns/artifact_registry/caches_client.rb:9), which holds the stubbed TokenExchange from ee/lib/artifact_registry/client.rb:78.

Review threads addressed

How to set up and validate locally

bundle exec rspec ee/spec/frontend/fixtures/artifact_registry/repository.rb

I could not run this locally: a gitaly from another checkout held port 9236, so the test gitaly could not bind. RuboCop is clean. Relying on CI for the spec.

References

MR acceptance checklist

  • No changelog: comments and specs only, nothing user-facing.
  • No documentation change: no behavior, API, or UI surface changes.
  • No screenshot: no visible UI change.

Merge request reports