Skip to content

Follow-up from "Gitlab housekeeper initial implementation with finalize BBM"

The following discussions from !139492 (merged) should be addressed:

  • @DylanGriffith started a discussion: (+1 comment)

    I wasn't actually sure whether we prefer to use add_development_dependency or if we add these to the Gemfile directly. I'm not actually sure what's the difference.

  • @tigerwnz started a discussion: (+1 comment)

    Tooling feels like the correct category in the long run, but maybe we should leave this out while these features are still in development.

  • @project_278964_bot_e50c5a74c24642c571054e7c8aedefc4 started a discussion: (+1 comment)

      gem 'gitlab-housekeeper', path: 'gems/gitlab-housekeeper' # rubocop:todo Gemfile/MissingFeatureCategory -- TODO: Reason why the rule must be disabled

    Consider removing this inline disabling and adhering to the rubocop rule.

    If that isn't possible, please provide the reason as a code comment in the same line where the rule is disabled separated by --. See rubocop best practices.


    Improve this message or have feedback?

  • @project_278964_bot_e50c5a74c24642c571054e7c8aedefc4 started a discussion:

        class Keep # rubocop:disable Lint/EmptyClass -- TODO: Reason why the rule must be disabled

    Consider removing this inline disabling and adhering to the rubocop rule.

    If that isn't possible, please provide the reason as a code comment in the same line where the rule is disabled separated by --. See rubocop best practices.


    Improve this message or have feedback?

  • @project_278964_bot_e50c5a74c24642c571054e7c8aedefc4 started a discussion:

        Dir.chdir(@previous_dir) if @previous_dir # rubocop:disable RSpec/InstanceVariable -- TODO: Reason why the rule must be disabled

    Consider removing this inline disabling and adhering to the rubocop rule.

    If that isn't possible, please provide the reason as a code comment in the same line where the rule is disabled separated by --. See rubocop best practices.


    Improve this message or have feedback?

  • @project_278964_bot_e50c5a74c24642c571054e7c8aedefc4 started a discussion:

      let(:fake_keep) { double(:fake_keep) } # rubocop:disable RSpec/VerifiedDoubles -- TODO: Reason why the rule must be disabled

    Consider removing this inline disabling and adhering to the rubocop rule.

    If that isn't possible, please provide the reason as a code comment in the same line where the rule is disabled separated by --. See rubocop best practices.


    Improve this message or have feedback?

  • @project_278964_bot_e50c5a74c24642c571054e7c8aedefc4 started a discussion:

        fake_keep_instance = double(:fake_keep_instance) # rubocop:disable RSpec/VerifiedDoubles -- TODO: Reason why the rule must be disabled

    Consider removing this inline disabling and adhering to the rubocop rule.

    If that isn't possible, please provide the reason as a code comment in the same line where the rule is disabled separated by --. See rubocop best practices.


    Improve this message or have feedback?

  • @tkuah started a discussion: (+1 comment)

    non-blocking: A quick observation here : Are each keep file be 200+ lines ? I wonder if this is a good pattern going forward.

    Also it might be worth having a comment at the top of this file, stating:

    • What this does (creates merge requests)
    • How to run it
  • @splattael started a discussion:

    require_relative '../config/environment'
  • @splattael started a discussion:

    Suggestion (non-blocking) Should we add def each; end and a bit of documentation to highlight the main interface/purpose of this class?

    Reading keep.new.each in !139492 (diffs) wasn't straight obvious to me that's the way of invoking a keep.

    Perhaps we could find a more intention-revealing method name instead of each? 🤷

  • @gitlab-bot started a discussion:

    👋 @splattael, thanks for approving this merge request.

    This is the first time the merge request has been approved. To ensure we don't only run predictive pipelines, and we don't break master, a new pipeline will be started shortly.

    Please wait for the pipeline to start before resolving this discussion and set auto-merge for the new pipeline. See merging a merge request for more details.

  • @project_278964_bot_e50c5a74c24642c571054e7c8aedefc4 started a discussion:

      gem 'gitlab-housekeeper', path: 'gems/gitlab-housekeeper' # rubocop:todo Gemfile/MissingFeatureCategory -- TODO: Reason why the rule must be disabled

    Consider removing this inline disabling and adhering to the rubocop rule.

    If that isn't possible, please provide the reason as a code comment in the same line where the rule is disabled separated by --. See rubocop best practices.


    Improve this message or have feedback?

Edited by Dylan Griffith