Change :store_designs_in_lfs feature to default on

The MR that this commit is part of
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/13894 contains
work that lets us enable this feature flag by default.

See
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/13389#note_177511855

https://gitlab.com/gitlab-org/gitlab-ee/issues/9490

app/services/lfs/file_transformer.rb

@@ -4,6 +4,14 @@ module Lfs
   # Usage: Calling `new_file` check to see if a file should be in LFS and
   #        return a transformed result with `content` and `encoding` to commit.
   #
+  #        The `repository` passed to the initializer can be a Repository or
+  #        a DesignManagement::Repository (an EE-specific class that inherits
+  #        from Repository).
+  #
+  #        The `repository_type` property will be one of the types named in
+  #        `Gitlab::GlRepository.types`, and is recorded on the `LfsObjectsProject`
+  #        in order to identify the repository location of the blob.
+  #
   #        For LFS an LfsObject linked to the project is stored and an LFS
   #        pointer returned. If the file isn't in LFS the untransformed content
   #        is returned to save in the commit.
@@ -52,7 +60,7 @@ module Lfs
     end
 
     def cached_attributes
-      @cached_attributes ||= Gitlab::Git::AttributesAtRefParser.new(repository, branch_name)
+      @cached_attributes ||= repository.attributes_at(branch_name)
     end
 
     # rubocop: disable CodeReuse/ActiveRecord

ee/app/models/design_management/repository.rb

@@ -2,11 +2,50 @@
 
 module DesignManagement
   class Repository < ::Repository
+    extend ::Gitlab::Utils::Override
+
+    # We define static git attributes for the design repository as this
+    # repository is entirely GitLab-managed rather than user-facing.
+    #
+    # Enable all uploaded files to be stored in LFS.
+    MANAGED_GIT_ATTRIBUTES = <<~GA.freeze
+      /#{DesignManagement.designs_directory}/* filter=lfs diff=lfs merge=lfs -text
+    GA
+
     def initialize(project)
       full_path = project.full_path + EE::Gitlab::GlRepository::DESIGN.path_suffix
       disk_path = project.disk_path + EE::Gitlab::GlRepository::DESIGN.path_suffix
 
       super(full_path, project, disk_path: disk_path, repo_type: EE::Gitlab::GlRepository::DESIGN)
     end
+
+    # Override of a method called on Repository instances but sent via
+    # method_missing to Gitlab::Git::Repository where it is defined
+    def info_attributes
+      @info_attributes ||= Gitlab::Git::AttributesParser.new(MANAGED_GIT_ATTRIBUTES)
+    end
+
+    # Override of a method called on Repository instances but sent via
+    # method_missing to Gitlab::Git::Repository where it is defined
+    def attributes(path)
+      info_attributes.attributes(path)
+    end
+
+    # Override of a method called on Repository instances but sent via
+    # method_missing to Gitlab::Git::Repository where it is defined
+    def gitattribute(path, name)
+      attributes(path)[name]
+    end
+
+    # Override of a method called on Repository instances but sent via
+    # method_missing to Gitlab::Git::Repository where it is defined
+    def attributes_at(_ref = nil)
+      info_attributes
+    end
+
+    override :copy_gitattributes
+    def copy_gitattributes(_ref = nil)
+      true
+    end
   end
 end

ee/app/models/ee/project.rb

@@ -574,12 +574,14 @@ module EE
     end
 
     def design_management_enabled?
+      # LFS and GraphQL are required for using Design Management
+      #
       # Checking both feature availability on the license, as well as the feature
       # flag, because we don't want to enable design_management by default on
       # on prem installs yet.
-      # GraphQL is also required for using Design Management
-      feature_available?(:design_management) && ::Feature.enabled?(:design_management, self) &&
-        ::Gitlab::Graphql.enabled?
+      lfs_enabled? && ::Gitlab::Graphql.enabled? &&
+        feature_available?(:design_management) &&
+        ::Feature.enabled?(:design_management, self)
     end
 
     def design_repository

ee/app/services/design_management/save_designs_service.rb

@@ -99,7 +99,7 @@ module DesignManagement
     end
 
     def file_content(file, full_path)
-      return file.to_io if Feature.disabled?(:store_designs_in_lfs)
+      return file.to_io if Feature.disabled?(:store_designs_in_lfs, default_enabled: true)
 
       transformer = Lfs::FileTransformer.new(project, repository, target_branch)
       transformer.new_file(full_path, file.to_io).content

ee/spec/controllers/projects/designs_controller_spec.rb

@@ -3,12 +3,14 @@
 require 'spec_helper'
 
 describe Projects::DesignsController do
+  include DesignManagementTestHelpers
+
   let(:project) { create(:project, :public) }
   let(:issue) { create(:issue, project: project) }
   let(:design) { create(:design, :with_file, issue: issue) }
 
   before do
-    stub_licensed_features(design_management: true)
+    enable_design_management
   end
 
   describe 'GET #show' do
@@ -31,7 +33,14 @@ describe Projects::DesignsController do
       expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
     end
 
-    it_behaves_like 'a controller that can serve LFS files' do
+    # Pass `skip_lfs_disabled_tests: true` to this shared example to disable
+    # the test scenarios for when LFS is disabled globally.
+    #
+    # When LFS is disabled then the design management feature also becomes disabled.
+    # When the feature is disabled, the `authorize :read_design` check within the
+    # controller will never authorize the user. Therefore #show will return a 403 and
+    # we cannot test the data that it serves.
+    it_behaves_like 'a controller that can serve LFS files', skip_lfs_disabled_tests: true do
       let(:design) { create(:design, :with_lfs_file, issue: issue) }
       let(:lfs_oid) { project.design_repository.blob_at('HEAD', design.full_path).lfs_oid }
       let(:filename) { design.filename }

ee/spec/features/projects/issues/design_management/user_paginates_designs_spec.rb

 require 'spec_helper'
 
 describe 'User paginates issue designs', :js do
+  include DesignManagementTestHelpers
+
   let(:project) { create(:project_empty_repo, :public) }
   let(:issue) { create(:issue, project: project) }
 
   before do
+    enable_design_management
+
     create(:design, :with_file, issue: issue, filename: 'world.png')
     create(:design, :with_file, issue: issue, filename: 'dk.png')
 
-    stub_licensed_features(design_management: true)
-
     visit project_issue_path(project, issue)
 
     click_link 'Designs'

ee/spec/features/projects/issues/design_management/user_permissions_upload_spec.rb

 require 'spec_helper'
 
 describe 'User design permissions', :js do
+  include DesignManagementTestHelpers
+
   let(:project) { create(:project_empty_repo, :public) }
   let(:issue) { create(:issue, project: project) }
 
   before do
-    stub_licensed_features(design_management: true)
+    enable_design_management
 
     visit project_issue_path(project, issue)
 

ee/spec/features/projects/issues/design_management/user_uploads_designs_spec.rb

 require 'spec_helper'
 
 describe 'User uploads new design', :js do
+  include DesignManagementTestHelpers
+
   let(:user) { project.owner }
   let(:project) { create(:project_empty_repo, :public) }
   let(:issue) { create(:issue, project: project) }
@@ -11,7 +13,7 @@ describe 'User uploads new design', :js do
 
   context "when the feature is available" do
     before do
-      stub_licensed_features(design_management: true)
+      enable_design_management
 
       visit project_issue_path(project, issue)
 
@@ -33,8 +35,6 @@ describe 'User uploads new design', :js do
 
   context 'when the feature is not available' do
     before do
-      stub_licensed_features(design_management: false)
-
       visit project_issue_path(project, issue)
     end
 

ee/spec/features/projects/issues/design_management/user_views_design_spec.rb

 require 'spec_helper'
 
 describe 'User views issue designs', :js do
+  include DesignManagementTestHelpers
+
   let(:project) { create(:project_empty_repo, :public) }
   let(:issue) { create(:issue, project: project) }
 
   before do
-    create(:design, :with_file, issue: issue, filename: 'world.png')
+    enable_design_management
 
-    stub_licensed_features(design_management: true)
+    create(:design, :with_file, issue: issue, filename: 'world.png')
 
     visit project_issue_path(project, issue)
 

ee/spec/features/projects/issues/design_management/user_views_designs_spec.rb

 require 'spec_helper'
 
 describe 'User views issue designs', :js do
+  include DesignManagementTestHelpers
+
   let(:project) { create(:project_empty_repo, :public) }
   let(:issue) { create(:issue, project: project) }
 
   before do
-    create(:design, :with_file, issue: issue, filename: 'world.png')
+    enable_design_management
 
-    stub_licensed_features(design_management: true)
+    create(:design, :with_file, issue: issue, filename: 'world.png')
 
     visit project_issue_path(project, issue)
 

ee/spec/graphql/mutations/design_management/upload_spec.rb

@@ -2,6 +2,8 @@
 require 'spec_helper'
 
 describe Mutations::DesignManagement::Upload do
+  include DesignManagementTestHelpers
+
   let(:issue) { create(:issue) }
   let(:user) { issue.author }
   let(:project) { issue.project }
@@ -24,16 +26,12 @@ describe Mutations::DesignManagement::Upload do
     end
 
     context "when the feature is not available" do
-      before do
-        stub_licensed_features(design_management: false)
-      end
-
       it_behaves_like "resource not available"
     end
 
     context "when the feature is available" do
       before do
-        stub_licensed_features(design_management: true)
+        enable_design_management
       end
 
       context "when the user is not allowed to upload designs" do

ee/spec/graphql/resolvers/design_management/version_resolver_spec.rb

@@ -2,9 +2,10 @@ require 'spec_helper'
 
 describe Resolvers::DesignManagement::VersionResolver do
   include GraphqlHelpers
+  include DesignManagementTestHelpers
 
   before do
-    stub_licensed_features(design_management: true)
+    enable_design_management
   end
 
   describe "#resolve" do

ee/spec/lib/gitlab/git_access_design_spec.rb

@@ -2,6 +2,8 @@
 require 'spec_helper'
 
 describe Gitlab::GitAccessDesign do
+  include DesignManagementTestHelpers
+
   set(:project) { create(:project) }
   set(:user) { project.owner }
   let(:protocol) { 'web' }
@@ -14,8 +16,7 @@ describe Gitlab::GitAccessDesign do
     subject { access.check('git-receive-pack', ::Gitlab::GitAccess::ANY) }
 
     before do
-      stub_licensed_features(design_management: true)
-      stub_feature_flags(design_managment: true)
+      enable_design_management
     end
 
     context "when the user is allowed to manage designs" do

ee/spec/models/design_management/repository_spec.rb

+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe DesignManagement::Repository do
+  let(:project) { create(:project) }
+  let(:repository) { described_class.new(project) }
+
+  shared_examples 'returns parsed git attributes that enable LFS for all file types' do
+    it do
+      expect(subject.patterns).to be_a_kind_of(Hash)
+      expect(subject.patterns).to have_key('/designs/*')
+      expect(subject.patterns['/designs/*']).to eql(
+        { "filter" => "lfs", "diff" => "lfs", "merge" => "lfs", "text" => false }
+      )
+    end
+  end
+
+  describe "#info_attributes" do
+    subject { repository.info_attributes }
+
+    include_examples 'returns parsed git attributes that enable LFS for all file types'
+  end
+
+  describe '#attributes_at' do
+    subject { repository.attributes_at }
+
+    include_examples 'returns parsed git attributes that enable LFS for all file types'
+  end
+
+  describe '#gitattribute' do
+    it 'returns a gitattribute when path has gitattributes' do
+      expect(repository.gitattribute('/designs/file.txt', 'filter')).to eq('lfs')
+    end
+
+    it 'returns nil when path has no gitattributes' do
+      expect(repository.gitattribute('/invalid/file.txt', 'filter')).to be_nil
+    end
+  end
+
+  describe '#copy_gitattributes' do
+    it 'always returns regardless of whether given a valid or invalid ref' do
+      expect(repository.copy_gitattributes('master')).to be true
+      expect(repository.copy_gitattributes('invalid')).to be true
+    end
+  end
+
+  describe '#attributes' do
+    it 'confirms that all files are LFS enabled' do
+      %w(png zip anything).each do |filetype|
+        path = "/#{DesignManagement.designs_directory}/file.#{filetype}"
+        attributes = repository.attributes(path)
+
+        expect(attributes['filter']).to eq('lfs')
+      end
+    end
+  end
+end

ee/spec/models/project_spec.rb

@@ -1830,18 +1830,20 @@ describe Project do
 
   describe "#design_management_enabled?" do
     let(:project) { build(:project) }
-    where(:feature_enabled, :license_enabled, :graphql, :expected) do
-      false | false | false | false
-      false | true  | false | false
-      true  | false | false | false
-      false | false | true  | false
-      true  | true  | true  | true
+    where(:feature_enabled, :license_enabled, :graphql, :lfs, :expected) do
+      false | false | false | false | false
+      true  | false | false | false | false
+      false | true  | false | false | false
+      false | false | true  | false | false
+      false | false | false | true | false
+      true  | true  | true | true | true
     end
 
     with_them do
       before do
         stub_licensed_features(design_management: license_enabled)
         stub_feature_flags(design_management: feature_enabled, graphql: graphql)
+        expect(project).to receive(:lfs_enabled?).and_return(lfs)
       end
 
       it "knows if design management is available" do

ee/spec/policies/design_management/design_policy_spec.rb

@@ -2,6 +2,8 @@
 require 'spec_helper'
 
 describe DesignManagement::DesignPolicy do
+  include DesignManagementTestHelpers
+
   include_context 'ProjectPolicy context'
 
   let(:guest_design_abilities) { %i[read_design] }
@@ -96,6 +98,7 @@ describe DesignManagement::DesignPolicy do
     before do
       stub_licensed_features(design_management: true)
       stub_feature_flags(design_management: false)
+      allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
     end
 
     it_behaves_like "design abilities not available"
@@ -105,15 +108,25 @@ describe DesignManagement::DesignPolicy do
     before do
       stub_licensed_features(design_management: false)
       stub_feature_flags(design_management: true)
+      allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
     end
 
     it_behaves_like "design abilities not available"
   end
 
-  context "when the feature is available" do
+  context "when LFS is not enabled" do
     before do
       stub_licensed_features(design_management: true)
       stub_feature_flags(design_management: true)
+      allow(Gitlab.config.lfs).to receive(:enabled).and_return(false)
+    end
+
+    it_behaves_like "design abilities not available"
+  end
+
+  context "when the feature is available" do
+    before do
+      enable_design_management
     end
 
     it_behaves_like "design abilities available for members"

ee/spec/requests/api/graphql/mutations/design_management/upload_spec.rb

@@ -3,6 +3,7 @@ require "spec_helper"
 
 describe "uploading designs" do
   include GraphqlHelpers
+  include DesignManagementTestHelpers
 
   let(:current_user) { create(:user) }
   let(:issue) { create(:issue) }
@@ -22,8 +23,7 @@ describe "uploading designs" do
   let(:mutation_response) { graphql_mutation_response(:design_management_upload) }
 
   before do
-    stub_feature_flags(design_mangement: true)
-    stub_licensed_features(design_management: true)
+    enable_design_management
 
     project.add_developer(current_user)
   end

ee/spec/requests/api/graphql/project/issue/designs/designs_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe "Getting designs related to an issue" do
   include GraphqlHelpers
+  include DesignManagementTestHelpers
 
   set(:design) { create(:design) }
   set(:current_user) { design.project.owner }
@@ -65,8 +66,7 @@ describe "Getting designs related to an issue" do
 
   context "when the feature is available" do
     before do
-      stub_licensed_features(design_management: true)
-      stub_feature_flags(deesign_managment: true)
+      enable_design_management
     end
 
     it "returns the design filename" do

ee/spec/requests/api/graphql/project/issue/designs/notes_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe 'Getting designs related to an issue' do
   include GraphqlHelpers
+  include DesignManagementTestHelpers
 
   set(:project) { create(:project, :public) }
   set(:issue) { create(:issue, project: project) }
@@ -12,8 +13,7 @@ describe 'Getting designs related to an issue' do
   set(:note) { create(:diff_note_on_design, noteable: design, project: project) }
 
   before do
-    stub_licensed_features(design_management: true)
-    stub_feature_flags(design_managment: true)
+    enable_design_management
 
     note
   end

ee/spec/services/lfs/file_transformer_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe Lfs::FileTransformer do
+  let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository }
+  let(:file_content) { 'Test file content' }
+  let(:branch_name) { 'lfs' }
+
+  subject { described_class.new(project, repository, branch_name) }
+
+  describe '#new_file' do
+    before do
+      allow(project).to receive(:lfs_enabled?).and_return(true)
+
+      file.write(file_content)
+      file.rewind
+    end
+
+    after do
+      file.unlink
+    end
+
+    context 'when repository is a design repository' do
+      let(:file_path) { "/#{DesignManagement.designs_directory}/test_file.lfs" }
+      let(:file) { Tempfile.new(file_path) }
+      let(:repository) { project.design_repository }
+
+      it "creates an LfsObject with the file's content" do
+        subject.new_file(file_path, file)
+
+        expect(LfsObject.last.file.read).to eq(file_content)
+      end
+    end
+  end
+end

ee/spec/support/helpers/design_management_test_helpers.rb

+# frozen_string_literal: true
+
+module DesignManagementTestHelpers
+  def enable_design_management
+    stub_licensed_features(design_management: true)
+    stub_lfs_setting(enabled: true)
+  end
+end

lib/gitlab/git/repository.rb

@@ -683,17 +683,16 @@ module Gitlab
         attributes(path)[name]
       end
 
-      # Check .gitattributes for a given ref
+      # Returns parsed .gitattributes for a given ref
       #
-      # This only checks the root .gitattributes file,
+      # This only parses the root .gitattributes file,
       # it does not traverse subfolders to find additional .gitattributes files
       #
       # This method is around 30 times slower than `attributes`, which uses
       # `$GIT_DIR/info/attributes`. Consider caching AttributesAtRefParser
       # and reusing that for multiple calls instead of this method.
-      def attributes_at(ref, file_path)
-        parser = AttributesAtRefParser.new(self, ref)
-        parser.attributes(file_path)
+      def attributes_at(ref)
+        AttributesAtRefParser.new(self, ref)
       end
 
       def languages(ref = nil)

spec/support/shared_examples/controllers/repository_lfs_file_load_examples.rb

@@ -9,7 +9,15 @@
 # - `filepath`: path of the file (contains filename)
 # - `subject`: the request to be made to the controller. Example:
 # subject { get :show, namespace_id: project.namespace, project_id: project }
-shared_examples 'a controller that can serve LFS files' do
+#
+# The LFS disabled scenario can be skipped by passing `skip_lfs_disabled_tests: true`
+# when including the examples (Note, at time of writing this is only used by
+# an EE-specific spec):
+#
+# it_behaves_like 'a controller that can serve LFS files', skip_lfs_disabled_tests: true do
+#   ...
+# end
+shared_examples 'a controller that can serve LFS files' do |options = {}|
   let(:lfs_oid) { '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897' }
   let(:lfs_size) { '1575078' }
   let!(:lfs_object) { create(:lfs_object, oid: lfs_oid, size: lfs_size) }
@@ -83,6 +91,8 @@ shared_examples 'a controller that can serve LFS files' do
     end
 
     it 'delivers ASCII file' do
+      skip 'Calling spec asked to skip testing LFS disabled scenario' if options[:skip_lfs_disabled_tests]
+
       subject
 
       expect(response).to have_gitlab_http_status(200)