Adding 'visible' field to custom attributes #14611

enoodle · 2017-04-03T08:42:13Z

The field's default value will be true to leave the behavior unchanged.

enoodle · 2017-04-03T08:45:01Z

@gmcculloug @moolitayer @simon3z @himdel

moolitayer · 2017-04-03T08:47:20Z

It makes more sense to me to have default false, as custom attributes are an internal implementation detail.

EDIT: this means you just omit the default. Lets wait to see what @gmcculloug and @himdel think

isimluk · 2017-04-03T11:28:49Z

Perfect. What is this trying to achieve?

enoodle · 2017-04-03T11:32:18Z

@isimluk Some times we don't want to display all Custom Attributes and we need an easy way to tell about a Custom Attribute if it should be displayed or not.

lpichler · 2017-04-03T11:33:39Z

what about default value ? isn`t it now nil ? is it ok ?

enoodle · 2017-04-03T11:36:29Z

@lpichler Yes, it is nil. It will not change anything for existing code that was ignoring this field before. If you would like to set a custom attribute visible you need to set this field to be 'true'.

moolitayer · 2017-04-03T11:38:32Z

Perfect. What is this trying to achieve?

Suggested here: ManageIQ/manageiq-ui-classic#290 (comment)

lpichler · 2017-04-03T11:46:51Z

@miq-bot assign @Fryguy

himdel · 2017-04-03T12:03:58Z

Please note that until now, all custom attributes with source=EVM are visible in the UI.

Therefore, defaulting visible to false definitely breaks everything ;) (if that visible is ever used :))

enoodle · 2017-04-03T12:08:39Z

@himdel
We could change the default back to true. This field is used here: ManageIQ/manageiq-ui-classic#881

moolitayer · 2017-04-03T12:11:40Z

Please note that until now, all custom attributes with source=EVM are visible in the UI.

Yes but as I understand we are adding the new column to switch to a general solution

Therefore, defaulting visible to false definitely breaks everything ;) (if that visible is ever used :))

existing code isn't looking at visible yet and should be converted to do so.
people accountable for relevant providers should convert their code.

lpichler · 2017-04-03T12:13:19Z

and isn't better to use column with name hidden with default value nil (false) ?

himdel · 2017-04-03T12:14:24Z

Agreed @enoodle no problem with this assuming visible deafults to true. Altough, I think the filtering on visible = true should be done in custom_attribte_mixin.rb instead of in the UI.

people accountable for relevant providers should convert their code.

@moolitayer That's IMHO exactly the wrong approach. The only way for us to treat custom attributes consistently is to exactly not let provider-specific people touch that code :). (That said, @enoodle's UI PR is fine IMHO :).)

gmcculloug · 2017-04-03T13:03:46Z

I agree with @himdel that the visible flag should default to true as this is used extensively today to show data on Services, VMs and other objects.

Two things are missing here:

The default_value_for to set this to true
A data migration (and spec) to set the field for existing custom_attribute instances.

@lpichler We tend to prefer the positive field name visible over the opposite. It simplifies the question "is something visible" or "should it be shown" instead of having to reverse the logic and ask "is something NOT hidden". And if you default the value you do not have to play the boolean = nil game.

Also need to expose setting more properties of the custom_attributes from automate but I would expect that in a separate PR.

lpichler · 2017-04-03T13:51:21Z

A data migration (and spec) to set the field for existing custom_attribute instances.

This data migration please to the separated migration.

gmcculloug · 2017-04-03T14:14:44Z

db/migrate/20170403135631_set_visible_to_trued_for_custom_attributes.rb

@@ -0,0 +1,9 @@
+class SetVisibleToTruedForCustomAttributes < ActiveRecord::Migration[5.0]
+  def up
+    CustomAttribute.where(:visible => nil).update_all(:visible => true)


We are adding the column so all rows would be nil. I do not see a need for where(:visible => nil)?

I realize someone could rollback to this migration without removing the column, but that seems highly unlikely and in my opinion more of a reason to include this in the migration that adds the column.

@lpichler What is the need to have this in a separate migration?

@gmcculloug I removed the where(:visible => nil).

gmcculloug · 2017-04-03T14:16:29Z

db/migrate/20170402131236_add_visible_field_for_custom_attributes.rb

@@ -0,0 +1,5 @@
+class AddVisibleFieldForCustomAttributes < ActiveRecord::Migration[5.0]
+  def change
+    add_column :custom_attributes, :visible, :boolean, default: true


Do we need the default here as well as the default_value_for? I would have expected just the default_value_for setting.

cc @Fryguy

@gmcculloug I removed the default: true from the migration itself.

miq-bot · 2017-04-03T16:47:55Z

Checked commit enoodle@00d5a92 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 1 offense detected

db/migrate/20170403135631_set_visible_to_true_for_custom_attributes.rb

❗ - Line 7, Col 21 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.

enoodle · 2017-04-24T11:17:43Z

@miq-bot add_label fine/no

can we take another look at this?

The default_value_for to set this to true
A data migration (and spec) to set the field for existing custom_attribute instances.

Added

gmcculloug · 2017-04-24T12:39:33Z

db/migrate/20170403135631_set_visible_to_true_for_custom_attributes.rb

+    CustomAttribute.update_all(:visible => true)
+  end
+
+  def down


@enoodle For future reference, there is no need to include the down method if it is empty.

@enoodle Yes, please remove the empty down migration.

simon3z · 2017-05-15T14:01:04Z

@enoodle can you give an example (probably related to containers) where you expect this set to false (not visible)? (Maybe I missed one of your messages on this)

simon3z

Per previous comment this needs an example/explanation.

chessbyte · 2017-06-02T03:57:01Z

@enoodle @simon3z bump

enoodle · 2017-06-04T14:01:37Z

@simon3z It can be used to prevent displaying of custom attributes in the UI, for example the http_proxy ones that are from "cluster_settings" section.

simon3z · 2017-06-06T17:26:04Z

@simon3z It can be used to prevent displaying of custom attributes in the UI, for example the http_proxy ones that are from "cluster_settings" section.

@enoodle didn't we say that visibility is by section? E.g. none of the cluster_settings sections should be visible.
The cluster_settings should have a specific configuration page.

enoodle · 2017-06-07T07:50:08Z

@simon3z we had ManageIQ/manageiq-ui-classic#290 to control the visibility by section but then the discussion lead to this PR. I can try to re-open this or make a new one with a more wide reach across all providers to prevent showing custom attributes from certain sections but I have the feeling that there is some resistance to it from the UI/Automate team.

himdel · 2017-06-07T13:33:19Z

👍 Visibility by section was abandoned because of the problems it would bring to all existing code and uses.
Per-attribute visiblity can be handled cleanly in the UI and won't affect existing users.
I'd say this is good to go..

simon3z · 2017-06-08T10:36:55Z

@himdel @enoodle I think there is some confusion around what we're really doing here.

Maybe at the end of the discussion this implementation will be OK but I feel that we're missing some background with a broader forum.

I'll organize a meeting. For me for the time being at least for the use-case I have this is 👎 .

himdel · 2017-06-08T12:43:46Z

@simon3z Please document your reasons and problems of this implementation in a textual form. If you need a meeting, it can happen after (I'm on PTO the rest of the week, sorry).

But the short story is, there's limited number of things you can do with custom attributes without affecting everybody else, and if you need to have a concept of hidden sections, then you'd need a concept of sections as an entity. Which means migrations, etc.

Per-attribute visibility is a general case that should cover any use case that would be solved by per-section visiblity, so .. please be specific about what your needs and constraints really are :).

Fryguy · 2017-06-13T17:51:29Z

db/migrate/20170403135631_set_visible_to_true_for_custom_attributes.rb

+  end
+
+  def up
+    CustomAttribute.update_all(:visible => true)


Please wrap this in a say_with_time as per the other data migrations.

Fryguy · 2017-06-13T17:54:54Z

Since ManageIQ/manageiq-ui-classic#290 is closed, should this be closed as well, or is a discussion still happening?

enoodle · 2017-06-15T14:28:56Z

@Fryguy No, this PR goes with ManageIQ/manageiq-ui-classic#881

simon3z · 2017-06-22T10:07:38Z

@himdel @enoodle after the last discussion I think we can close this.

himdel · 2017-06-22T11:08:12Z

@simon3z I guess so.. can you please summarize the result of that discussion? (I had to leave the meeting before it ended, so.. not sure which solution you settled on in the end..)

simon3z · 2017-06-22T14:09:22Z

@simon3z I guess so.. can you please summarize the result of that discussion? (I had to leave the meeting before it ended, so.. not sure which solution you settled on in the end..)

@himdel the discussion resulted in #15398

miq-bot · 2017-06-22T17:37:30Z

This pull request is not mergeable. Please rebase and repush.

Fryguy · 2017-06-22T17:41:43Z

Database migrations have now been moved to the https://github.com/ManageIQ/manageiq-schema repo. Please see http://talk.manageiq.org/t/new-split-repo-manageiq-schema/2478 for instructions on how to transfer your database migrations. If this PR contains only migrations, I will leave it open for a short time during the transition, after which I will close this if it has not been moved over.

enoodle mentioned this pull request Apr 3, 2017

Only show visible custom attributes ManageIQ/manageiq-ui-classic#881

Closed

miq-bot added the sql migration label Apr 3, 2017

enoodle force-pushed the visible_field_for_custom_attributes branch from c9da002 to 688b15e Compare April 3, 2017 09:33

miq-bot assigned Fryguy Apr 3, 2017

enoodle force-pushed the visible_field_for_custom_attributes branch 2 times, most recently from 7ae0064 to 72a9f77 Compare April 3, 2017 14:09

gmcculloug reviewed Apr 3, 2017

View reviewed changes

enoodle force-pushed the visible_field_for_custom_attributes branch 2 times, most recently from 54a72cd to 3efe49b Compare April 3, 2017 14:41

adding 'visible' field to custom attributes

00d5a92

enoodle force-pushed the visible_field_for_custom_attributes branch from 3efe49b to 00d5a92 Compare April 3, 2017 16:41

miq-bot added the fine/no label Apr 24, 2017

gmcculloug reviewed Apr 24, 2017

View reviewed changes

gmcculloug approved these changes Apr 24, 2017

View reviewed changes

simon3z suggested changes May 17, 2017

View reviewed changes

Fryguy reviewed Jun 13, 2017

View reviewed changes

miq-bot added the unmergeable label Jun 22, 2017

enoodle closed this Jun 25, 2017

Adding 'visible' field to custom attributes #14611

Adding 'visible' field to custom attributes #14611

Conversation

enoodle commented Apr 3, 2017 • edited Loading

enoodle commented Apr 3, 2017

moolitayer commented Apr 3, 2017 • edited Loading

isimluk commented Apr 3, 2017

enoodle commented Apr 3, 2017

lpichler commented Apr 3, 2017

enoodle commented Apr 3, 2017

moolitayer commented Apr 3, 2017

lpichler commented Apr 3, 2017

himdel commented Apr 3, 2017 • edited Loading

enoodle commented Apr 3, 2017

moolitayer commented Apr 3, 2017

lpichler commented Apr 3, 2017

himdel commented Apr 3, 2017

gmcculloug commented Apr 3, 2017

lpichler commented Apr 3, 2017 • edited Loading

gmcculloug Apr 3, 2017

Choose a reason for hiding this comment

enoodle Apr 3, 2017

Choose a reason for hiding this comment

gmcculloug Apr 3, 2017

Choose a reason for hiding this comment

enoodle Apr 3, 2017

Choose a reason for hiding this comment

miq-bot commented Apr 3, 2017

enoodle commented Apr 24, 2017 • edited Loading

gmcculloug Apr 24, 2017

Choose a reason for hiding this comment

Fryguy Jun 13, 2017

Choose a reason for hiding this comment

simon3z commented May 15, 2017

simon3z left a comment

Choose a reason for hiding this comment

chessbyte commented Jun 2, 2017

enoodle commented Jun 4, 2017

simon3z commented Jun 6, 2017

enoodle commented Jun 7, 2017

himdel commented Jun 7, 2017

simon3z commented Jun 8, 2017

himdel commented Jun 8, 2017

Fryguy Jun 13, 2017

Choose a reason for hiding this comment

Fryguy commented Jun 13, 2017

enoodle commented Jun 15, 2017

simon3z commented Jun 22, 2017

himdel commented Jun 22, 2017

simon3z commented Jun 22, 2017

miq-bot commented Jun 22, 2017

Fryguy commented Jun 22, 2017

enoodle commented Apr 3, 2017 •

edited

Loading

moolitayer commented Apr 3, 2017 •

edited

Loading

himdel commented Apr 3, 2017 •

edited

Loading

lpichler commented Apr 3, 2017 •

edited

Loading

enoodle commented Apr 24, 2017 •

edited

Loading