Reduce the responsibilities of ComponentVersions
Addresses technical debt outlined in https://gitlab.com/gitlab-org/release-tools/-/issues/389 for the ComponentVersions
class, such that the class will better adhere to the Single Responsibility Principle, and its functionality will better match its name, i.e., it gets the component versions in gitlab-rails, with methods for returning them in formats that Omnibus and CNG work with.
For both Builder::Omnibus
and Builder::CNGImage
, the refactoring roughly followed this process:
-
Move the methods exactly as-is from
ComponentVersions
, along with specs -
Make the methods instance methods rather than class methods
-
Remove the type-specific naming (e.g.,
update_cng
became justupdate
) -
Remove the branch argument, relying on the internal instance variable provided to the class's initializer
-
Remove the
version_map
argument, relying on the internalattr_reader
and the value assigned duringexecute
-
Remove the
update_*
method as it was basically a wrapper for this very simple logic that was moved directly into theexecute
method:if changes? commit else # log end
The CNG-related logic saw additional refactoring because we wound up fetching and parsing the YAML variable definitions in two places that wasn't necessary to begin with (see here and here).
All of this greatly reduced the number of necessary specs, due to making the "helper" methods internal private methods. Because all we need to end up testing is:
- Does the class fetch the versions from
ComponentVersions
formatted for its respective builder? - When those versions are identical to what's defined in the builder, do we do nothing?
- When those versions differ from what's defined in the builder, do we commit the changes?
So while this MR shows a bunch of deleted tests, we're still covering the same amount of functionality.