Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include machine learning settings type #4827

Merged
merged 2 commits into from
Jun 1, 2022
Merged

Conversation

decabeza
Copy link
Collaborator

@decabeza decabeza commented May 4, 2022

Description

On the Configuration settings page three settings appear without description:

  • Comments Summary: No description.
  • Related Content: No description.
  • Tags: No description.

These settings are related with the AI / Machine learning feature. They only should appear on AI / Machine learning setting page when the feature is enabled. 馃

This PR include machine learning settings type.

Notes

Actually adding the machine_learning type prevents them from being added to the configuration list in models/setting.rb, but as that type is not being used, maybe there is another way to exclude them. 馃

Visual changes

Settings removed from Configuration settings list

settings

@decabeza decabeza added the Admin label May 4, 2022
@decabeza decabeza self-assigned this May 4, 2022
@javierm javierm added the Bug label May 5, 2022
@javierm javierm added this to Reviewing in Consul Democracy via automation May 5, 2022
@javierm javierm self-assigned this May 5, 2022
It looks like these lines were added on a branch which didn't include
commit 3da4ee0 but were merged after that commit was merged.

In any case, since we're already using the `:admin` tag in the whole
file, these lines aren't necessary.
@javierm javierm force-pushed the machine_learning_settings branch 2 times, most recently from 3b09793 to a89e15b Compare May 5, 2022 16:26
On the Configuration settings page three settings appeared without
description:

* Comments Summary: No description.
* Related Content: No description.
* Tags: No description.

These settings are related with the AI / Machine learning feature. They
only should appear on AI / Machine learning setting page when the
feature is enabled.
@javierm javierm force-pushed the machine_learning_settings branch from a89e15b to 682781c Compare May 5, 2022 16:31
Copy link
Member

@javierm javierm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! 馃帀

I'm fine with adding machine_learning as a new type 馃憣, even if we don't use it. Maybe we could use it instead of the MachineLearning#script_kinds method 馃, but that's a different story 馃槃.

Consul Democracy automation moved this from Reviewing to Testing May 5, 2022
@taitus taitus self-assigned this Jun 1, 2022
@taitus
Copy link
Member

taitus commented Jun 1, 2022

It works fine in the testing server and the spec that fails is not related to PR and looks like a flaky.

@taitus taitus merged commit a1186ff into master Jun 1, 2022
Consul Democracy automation moved this from Testing to Release 1.5.0 Jun 1, 2022
@taitus taitus deleted the machine_learning_settings branch June 1, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants