[RFE] Use `config/settings*` for configuring "secrets", but not "configs"

NickLaMuro commented 4 years ago

Discussion spawned from https://github.com/ManageIQ/miq_bot/pull/485#issuecomment-598452344

A note on my previous confusion

I want to clear up that part of this issue was opened up based on a incorrect assumption about how included_repos/excluded_repos worked:

https://github.com/ManageIQ/miq_bot/blob/ca52db9/README.md#enabling-and-disabling-workers

Which I was under the understanding is that this was used to disable what repos are monitored by the bot overall, not what repos are monitored by specific workers in a multi-worker-type environment. I think my points about this still are valid, but wanted to mention this prior in case this wasn't clear to anyone else.

Rational

There are items in the config/settings* that are worth being made public for viability for contributors that don't have server access, specifically the newly added labels: section.

Suggestion

Create a new configuration setting config_repo: that accepts a git URL that can contain one or many yaml files that can be merged into the existing Settings object.

This way, it allows public discussion when something is a configuration issue for the bot for things that don't need to remain secret. We could add it as part of ManageIQ/guides, or create a new repo, for example miq_bot_config.

In addition, it might not hurt have some seed files in that repo for the bot to handle what Repo objects are being added, and it would allow other users to suggest what repos should be watched or at least be aware of what currently being monitored by the bot.

NickLaMuro commented 4 years ago

This is in a similar vein to https://github.com/ManageIQ/miq_bot/issues/484 but a bit of a different subject matter. Could arguably be merged into one issue.

bdunne commented 4 years ago

Create a new configuration setting config_repo: that accepts a git URL

Interesting idea. I could see some of this stuff possibly living in the guides repo where we store the default rubocop configs.

This way, it allows public discussion when something is a configuration issue for the bot

I'm fine with making everything except for the secrets publicly visible. I don't think there's anything to discuss in there, but ¯\_(ツ)_/¯

bdunne commented 4 years ago

FWIW, here's the currently running config

---
# Credentials
bugzilla_credentials:
  url: https://bugzilla.redhat.com
  username: REDACTED
  password: REDACTED
github_credentials:
  username: REDACTED
  password: REDACTED
pivotal_credentials:
  token: REDACTED
influxdb_credentials:
  database: REDACTED
  username: REDACTED
  password: REDACTED

# General settings
grafana:
  url: REDACTED
bugzilla:
  product: Red Hat CloudForms Management Engine

# Core repos
core_repos: &core_repos
- ManageIQ/manageiq
- ManageIQ/manageiq-api
- ManageIQ/manageiq-api-mock
- ManageIQ/manageiq-appliance
- ManageIQ/manageiq-appliance-build
- ManageIQ/manageiq-automation_engine
- ManageIQ/manageiq-consumption
- ManageIQ/manageiq-content
- ManageIQ/manageiq-gems-pending
- ManageIQ/manageiq-graphql
- ManageIQ/manageiq-pods
- ManageIQ/manageiq-providers-amazon
- ManageIQ/manageiq-providers-ansible_tower
- ManageIQ/manageiq-providers-azure
- ManageIQ/manageiq-providers-azure_stack
- ManageIQ/manageiq-providers-foreman
- ManageIQ/manageiq-providers-google
- ManageIQ/manageiq-providers-hawkular
- ManageIQ/manageiq-providers-kubernetes
- ManageIQ/manageiq-providers-kubevirt
- ManageIQ/manageiq-providers-lenovo
- ManageIQ/manageiq-providers-nuage
- ManageIQ/manageiq-providers-openshift
- ManageIQ/manageiq-providers-openstack
- ManageIQ/manageiq-providers-ovirt
- ManageIQ/manageiq-providers-redfish
- ManageIQ/manageiq-providers-scvmm
- ManageIQ/manageiq-providers-vmware
- ManageIQ/manageiq-schema
- ManageIQ/manageiq-ui-classic
- ManageIQ/manageiq-ui-service
- ManageIQ/manageiq-v2v
- ManageIQ/manageiq_docs

# Path Based Labeling rules
embedded_ansible_rule: &embedded_ansible_rule
  regex: !ruby/regexp /\/embedded_ansible/
  label: core/embedded_ansible
dependencies_rule: &dependencies_rule
  regex: !ruby/regexp /(?:Gemfile|Gemfile\.lock|\.gemspec)\z/
  label: dependencies
graphics_rule: &graphics_rule
  regex: !ruby/regexp /\.(?:png|svg|jpe?g|gif)\z/
  label: graphics
sql_migration_rule: &sql_migration_rule
  regex: !ruby/regexp /db\/migrate.+\.rb\z/
  label: sql migration

# Worker settings
diff_content_checker:
  offenses:
    "^([^#]+|)\\bputs\\b":
      type: :regexp
      severity: :warn
      message: Detected `puts`. Remove all debugging statements.
      except:
      - bin/
      - tools/
    pp:
      severity: :warn
      message: Detected `pp`. Remove all debugging statements.
      except:
      - tools/
    cfme:
      severity: :error
      except:
      - spec/models/manageiq/providers/microsoft/infra_manager/refresher_spec.rb
      - spec/tools/
    cloudforms:
      severity: :error
    byebug:
      severity: :error
      message: Detected `byebug`. Remove all debugging statements.
    binding.pry:
      severity: :error
      message: Detected `binding.pry`. Remove all debugging statements.
    allow_any_instance_of:
      severity: :warn
      message: Detected `allow_any_instance_of`. This RSpec method is highly discouraged, please only use when absolutely necessary.
    expect_any_instance_of:
      severity: :warn
      message: Detected `expect_any_instance_of`. This RSpec method is highly discouraged, please only use when absolutely necessary.
    "it ['\"](works|stuff|things)['\"] do":
      type: :regexp
      severity: :warn
      message: Detected unoriginal RSpec message. Please explain the spec purpose in more detail.
merge_target_titler:
  included_repos: *core_repos
path_based_labeler:
  included_repos:
  - ManageIQ/manageiq
  - ManageIQ/manageiq-ui-classic
  rules:
    ManageIQ/manageiq:
    - *dependencies_rule
    - *embedded_ansible_rule
    - *sql_migration_rule
    ManageIQ/manageiq-ui-classic:
    - *graphics_rule
    - *dependencies_rule
travis_build_killer:
  included_repos: []

NickLaMuro commented 4 years ago

Oh, diff_content_checker: was one that I wasn't even considering. That is another good one I would think that would be nice to have in the open.

And I know from personal experience that there has been "discussion" around that topic:

https://github.com/ManageIQ/miq_bot/pull/426

😄

Fryguy commented 4 years ago

I think I'd like it the other way around from the title. That is, store default values in config (perhaps on config/environments/production.yml), and keep secrets in Rails' config/secrets.yml (now that we are on 5.2 we can use that).

From a configuration perspective, though, it's weird to put "repos" into the settings when they aren't in the database. That is, we add repos to the database on the fly, and then add the configuration afterwards. Having them in configuration first seems backwards. That being said, maybe the list of repos being monitored can also be config driven. Then it won't be so weird because they are predefined already.

NickLaMuro commented 4 years ago

I think I'd like it the other way around from the title. That is, store default values in config (perhaps on config/environments/production.yml), and keep secrets in Rails' config/secrets.yml (now that we are on 5.2 we can use that).

I can get behind that for an idea. Not sure what it would take, but maybe having config favor config/environments/*.local.yml over what is in the repo will allow developers to use that for development, or we just have a development.yml.example that can be configured on clone and development.yml is not checked in.

From a configuration perspective, though, it's weird to put "repos" into the settings when they aren't in the database.

Yeah, I did mention that was a miss-interpretation on my part in the description, but yes I agree putting this in the configs might be a bit weird, if not wrong all together. That said, there are conversations like this that happen:

https://github.com/ManageIQ/miq_bot/pull/469#discussion_r394047555

Where folks who aren't running the bot in production have no insight into this information what so ever, where having it as a seeds.rb file, data migration, or something similar would at least make this visible to the public and developers looking to understand how the bot should behave without requiring production access to do so. It could also easily be updated as a pull request, and even automated as part of a release/new_repo script in a similar vain as dependabot is currently.

That all said, I definitely see this "RFE" as something that is fluid and more of a discussion, along with this one, for ideas to improve in no way meant to be a "this is how is should be done" decree, as I am sure I am missing some necessary details.

ManageIQ / miq_bot