Follow-up from ROP part 3 - split up classes into smaller methods

MR: Pending

Description

The following discussion from !125358 (merged) should be addressed:

• @a_akgun started a discussion: (+3 comments)

  optional:

  @cwoolley-gitlab we could divide the create method into more methods, for example we could move some of the stuff here into a private method, like workspace_params etc.

  Similar concern for all other ee/lib/remote_development/workspaces/create/devfile_fetcher.rb

Potential Concerns

Follow-up comment from @cwoolley-gitlab :

---

It would end up looking like the following, which makes for a smaller entry point method, but makes us pass around a lot of arguments for the variables we destructure out of the value arg. I just returned them from destructure_value as an array, which is order-dependent - we could make them a hash if we wanted, but then we'd have to destructure them out of that hash again to use them in build_workspace, which seems excessive.

module RemoteDevelopment
  module Workspaces
    module Create
      # noinspection RubyResolve - Rubymine isn't detecting ActiveRecord db field properties of workspace
      class Creator
        include States
        include Messages

        # @param [Hash] value
        # @return [Result]
        def self.create(value)
          agent, devfile_yaml, params, processed_devfile, project, user, workspace_root = workspace_params(value)

          workspace = build_workspace(
            agent: agent,
            devfile_yaml: devfile_yaml,
            params: params,
            processed_devfile: processed_devfile,
            project: project,
            user: user,
            workspace_root: workspace_root
          )

          if workspace.save
            Result.ok(
              WorkspaceCreateSuccessful.new(
                value.merge({
                  workspace: workspace
                })
              )
            )
          else
            Result.err(WorkspaceCreateFailed.new({ errors: workspace.errors }))
          end
        end

        # @param [Clusters::Agent] agent
        # @param [String] devfile_yaml
        # @param [Hash] params
        # @param [Hash] processed_devfile
        # @param [Project] project
        # @param [User] user
        # @param [String] workspace_root
        # @return [Workspace]
        def self.build_workspace(agent:, devfile_yaml:, params:, processed_devfile:, project:, user:, workspace_root:)
          processed_devfile_yaml = YAML.dump(processed_devfile.deep_stringify_keys)

          workspace = project.workspaces.build(params)

          workspace.devfile = devfile_yaml
          workspace.processed_devfile = processed_devfile_yaml
          workspace.actual_state = CREATION_REQUESTED

          random_string = SecureRandom.alphanumeric(6).downcase
          workspace.namespace = "gl-rd-ns-#{agent.id}-#{user.id}-#{random_string}"
          # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/409774
          #       We can come maybe come up with a better/cooler way to get a unique name, for now this works
          workspace.name = "workspace-#{agent.id}-#{user.id}-#{random_string}"

          project_dir = "#{workspace_root}/#{project.path}"
          workspace_host = "60001-#{workspace.name}.#{workspace.dns_zone}"
          workspace.url = URI::HTTPS.build({
            host: workspace_host,
            query: {
              folder: project_dir
            }.to_query
          }).to_s
          workspace
        end

        # @param [Hash] value
        # @return [Array]
        def self.workspace_params(value)
          value => {
            devfile_yaml: String => devfile_yaml,
            processed_devfile: Hash => processed_devfile,
            volume_mounts: Hash => volume_mounts,
            current_user: User => user,
            params: Hash => params,
          }
          volume_mounts => { data_volume: Hash => data_volume }
          data_volume => {
            path: String => workspace_root,
          }
          params => {
            project: Project => project,
            agent: Clusters::Agent => agent,
          }

          [agent, devfile_yaml, params, processed_devfile, project, user, workspace_root]
        end
      end
    end
  end
end

Is this what you had in mind?

Ordinarily I'm all for extracting things to small, well-named, cohesive methods, but in this case, I'm not sure if this makes the code any more understandable.

Agreed Solution

Based on the thread below, we will create private methods where it makes sense to extract logic, but NOT for the value-destructuring-to-local-variables logic.

Acceptance Criteria

• Existing ROP classes are split into smaller methods

Impact Assessment

Makes it easier to understand the code.