Hide deprecated and duplicate attributes #20664

kbrock · 2020-10-07T00:06:33Z

Hide deprecated and duplicate attributes

Some virtual_attributes are not needed in reporting nor the api.
Some deprecated attributes should not be advertised for customers to start using.

Now, the attributes are still accessible. So nothing will break. But
these attributes are no longer advertised on the public apis (i.e.: report builder and rest api).

This allows us to control which parts of our models are exposed.
So it will fix #18130 and also allows us to introduce attributes in PRs like #20532 because we will not need to expose the newly introduced attribute.

This does modify the report builder, but minimal changes over in the api repo are necessary to plug up this hole over there.

d-m-u

thanks, this is excellent, it looks perfectly cromulent

NickLaMuro

Just some thoughts about making this a bit more robust and avoid some smells. I think the concept is fine though, so feel free to push back if you think these suggestions are invalid.

lib/miq_expression.rb

kbrock · 2020-10-09T02:06:51Z

I think the preferred way to do this is to use private instead of adding yet more metadata - but I felt like turning all these fields private has some unknown consequences

lib/miq_expression.rb

spec/lib/extensions/ar_visible_attribute_spec.rb

Fryguy · 2020-10-12T19:28:56Z

I like this idea... seems like a decent compromise over straight private. In that sense, can we name it private_attribute or perhaps even better private_virtual_attribute (since I assume we'll never want to hide regular attributes - though password does come to mind 😉 )

d-m-u · 2020-10-15T04:27:14Z

yea so models that inherit from the ActsAsArModel aren't going to work with this as it is now

lib/extensions/ar_visible_attribute.rb

spec/lib/extensions/ar_visible_attribute_spec.rb

NickLaMuro · 2020-11-16T16:15:05Z

Just pushed whitespace fix...

This doesn't sound like a just a whitespace fix...

NickLaMuro

Looks good to me. Took a peek at the tests, which look good and I think are a solid/clever way of testing without mucking with "un-privatizing" methods after each test.

Regarding Jason's feedback, I will leave that discussion to him. I can see both sides of that argument, so I don't have a strong opinion one way or the other.

NickLaMuro · 2020-11-16T16:17:43Z

lib/miq_expression.rb

@@ -909,7 +909,7 @@ def self.build_relats(model, parent = {}, seen = [])
    parent[:class_path] ||= model.name
    parent[:assoc_path] ||= model.name
    parent[:root] ||= model.name
-    result = {:columns => model.attribute_names, :parent => parent}
+    result = {:columns => model.public_attribute_names, :parent => parent}


The public_attribute_names is a good addition here, and really cleaned this up. 👍

bdunne

I don't love the name private_attribute because it makes me think that the attribute will act like a private method (unusable to public callers), maybe hidden_attribute would be better? Or built into deprecate_attribute as deprecated_attributes on the class? 🤷‍♂️

Overall, I like the idea of having a way to list the non-deprecated (public) attributes on a model. 👍

kbrock · 2020-11-16T21:04:32Z

maybe hidden_attribute would be better?
-- @bdunne

<3 I agree and that is the original name. [ref]

But I just want the thing merged and am good with any name that people pick.
(the branch name also suggests my agreement with your statement)

kbrock · 2020-11-17T16:03:37Z

I had this in deprecated attribute mixin. But the original intent was to hide new virtual attributes

kbrock · 2020-11-17T17:43:13Z

@miq-bot cross-repo-tests #20664

From Pull Request: ManageIQ/manageiq#20664

kbrock · 2020-11-17T18:05:32Z

for this pr, lets test across many repos (trying to get a handle on how this bot stuff works)

@miq-bot cross-repo-tests manageiq-ui-classic, manageiq-api, ManageIQ/manageiq-providers-amazon, ManageIQ/manageiq-providers-azure, ManageIQ/manageiq-providers-ibm_cloud, ManageIQ/manageiq-providers-kubernetes, ManageIQ/manageiq-providers-openstack, ManageIQ/manageiq-providers-vmware

From Pull Request: ManageIQ/manageiq#20664

Some virtual_attributes are not needed in reporting nor the api. Some deprecated attributes should not be advertised to the customer as attributes to use. After this change, the attributes are still accessible. Just these attributes are no longer advertised on the public apis (i.e.: report builder and rest api). This allows us to control which parts of our models are exposed. This fixes ManageIQ#18130 This enables ManageIQ#20532 Hide deprecated and duplicate attributes. Some virtual_attributes are not needed in reporting nor the api. Some deprecated attributes should not be advertised for customers to use. After this change, the attributes are still accessible. So nothing will break, but these attributes are no longer advertised in the public apis (i.e.: report builder and rest api). This allows us to control which parts of our models are exposed. This fixes ManageIQ#18130 This enables ManageIQ#20532

kbrock · 2020-12-08T15:40:38Z

status: cross repo failure was because this PR is against an older version of the code base
Just pushed a rebase and those should be fixed

kbrock · 2020-12-08T15:41:04Z

@miq-bot cross-repo-tests manageiq-ui-classic, manageiq-api, ManageIQ/manageiq-providers-amazon, ManageIQ/manageiq-providers-azure, ManageIQ/manageiq-providers-ibm_cloud, ManageIQ/manageiq-providers-kubernetes, ManageIQ/manageiq-providers-openstack, ManageIQ/manageiq-providers-vmware

From Pull Request: ManageIQ/manageiq#20664

Fryguy · 2020-12-08T15:48:37Z

I had this in deprecated attribute mixin. But the original intent was to hide new virtual attributes

Ahhhh...my original intent was for it to act more like private (ref & ref). This is probably why I was confused here.

However, I'm good if your intent is to act more like hidden. If that's the case, then I agree that hidden_attribute makes more sense as a name.

EDIT: Apologies for the super-late reply (I do realize my comment makes some annoying changes)

kbrock · 2020-12-08T19:11:33Z

right about now is when I wish my git-fu were not as good and I had an earlier commit with the hidden word in it (rather than rebasing and the changes I made)

coming right up

UPDATE: huh. for some reason I had the rename as a separate commit. Only had to add the fixes to this one (for the children tests).

miq-bot · 2020-12-08T19:50:37Z

Checked commits kbrock/manageiq@a926389~...151b057 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
8 files checked, 0 offenses detected
Everything looks fine. 👍

kbrock · 2020-12-08T20:04:38Z

@bdunne and @Fryguy is this closer to what we want?

bdunne

👍 LGTM

Fryguy · 2020-12-08T22:40:52Z

cc @agrare ... wonder if we can start using this to hide the support features stuff.

kbrock added api core/reporting labels Oct 7, 2020

kbrock requested review from agrare, Fryguy and gtanzillo as code owners October 7, 2020 00:06

kbrock force-pushed the hidden_attributes branch from c5395ba to 1cbca13 Compare October 7, 2020 19:15

d-m-u approved these changes Oct 7, 2020

View reviewed changes

kbrock requested a review from bdunne October 8, 2020 00:45

gtanzillo added the enhancement label Oct 8, 2020

gtanzillo assigned NickLaMuro Oct 8, 2020

NickLaMuro suggested changes Oct 8, 2020

View reviewed changes

lib/miq_expression.rb Outdated Show resolved Hide resolved

kbrock force-pushed the hidden_attributes branch 2 times, most recently from 9022920 to e9aaf79 Compare October 12, 2020 17:34

Fryguy reviewed Oct 12, 2020

View reviewed changes

lib/miq_expression.rb Outdated Show resolved Hide resolved

Fryguy reviewed Oct 12, 2020

View reviewed changes

spec/lib/extensions/ar_visible_attribute_spec.rb Show resolved Hide resolved

kbrock force-pushed the hidden_attributes branch 2 times, most recently from c0f9097 to 35c92d3 Compare October 23, 2020 23:59

d-m-u reviewed Oct 24, 2020

View reviewed changes

lib/extensions/ar_visible_attribute.rb Outdated Show resolved Hide resolved

d-m-u mentioned this pull request Oct 25, 2020

add binding to determine which method to use in relationship mixin #20274

Merged

d-m-u reviewed Nov 1, 2020

View reviewed changes

kbrock force-pushed the hidden_attributes branch from 35c92d3 to 67b6ce4 Compare November 2, 2020 19:00

kbrock requested a review from NickLaMuro November 2, 2020 20:50

kbrock force-pushed the hidden_attributes branch 2 times, most recently from a05aaf4 to 9dec939 Compare November 3, 2020 17:05

This was referenced Nov 6, 2020

replace cattr_accessor in rel mixin with a class_attribute #20785

Merged

prefer class attribute over cattr_accessor in miq_policy #20790

Merged

kbrock force-pushed the hidden_attributes branch from 507d42a to 6bb6b4b Compare November 9, 2020 20:37

kbrock mentioned this pull request Nov 13, 2020

introduce User.lower_userid to remove some arel across a few repos #20532

Merged

NickLaMuro approved these changes Nov 16, 2020

View reviewed changes

bdunne approved these changes Nov 16, 2020

View reviewed changes

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Nov 17, 2020

Running tests for kbrock

8fe69b5

From Pull Request: ManageIQ/manageiq#20664

miq-bot mentioned this pull request Nov 17, 2020

[BOT] Cross repo test for ManageIQ/manageiq#20664 ManageIQ/manageiq-cross_repo-tests#236

Closed

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Nov 17, 2020

Running tests for kbrock

fd37936

From Pull Request: ManageIQ/manageiq#20664

miq-bot mentioned this pull request Nov 17, 2020

[BOT] Cross repo test for ManageIQ/manageiq#20664 ManageIQ/manageiq-cross_repo-tests#237

Closed

kbrock force-pushed the hidden_attributes branch from 4e8f58f to 79e3e33 Compare December 8, 2020 15:40

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Dec 8, 2020

Running tests for kbrock

6c08953

From Pull Request: ManageIQ/manageiq#20664

miq-bot mentioned this pull request Dec 8, 2020

[BOT] Cross repo test for ManageIQ/manageiq#20664 ManageIQ/manageiq-cross_repo-tests#242

Closed

Add tests to ensure child/STI classes are hidden correctly

151b057

kbrock force-pushed the hidden_attributes branch from 79e3e33 to 151b057 Compare December 8, 2020 19:49

bdunne approved these changes Dec 8, 2020

View reviewed changes

bdunne merged commit a571d97 into ManageIQ:master Dec 8, 2020

bdunne assigned bdunne and unassigned NickLaMuro Dec 8, 2020

kbrock deleted the hidden_attributes branch December 8, 2020 23:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide deprecated and duplicate attributes #20664

Hide deprecated and duplicate attributes #20664

kbrock commented Oct 7, 2020

d-m-u left a comment

NickLaMuro left a comment

kbrock commented Oct 9, 2020

Fryguy commented Oct 12, 2020

d-m-u commented Oct 15, 2020

NickLaMuro commented Nov 16, 2020 •

edited

Loading

NickLaMuro left a comment •

edited

Loading

NickLaMuro Nov 16, 2020

bdunne left a comment

kbrock commented Nov 16, 2020 •

edited

Loading

kbrock commented Nov 17, 2020

kbrock commented Nov 17, 2020

kbrock commented Nov 17, 2020 •

edited

Loading

kbrock commented Dec 8, 2020

kbrock commented Dec 8, 2020

Fryguy commented Dec 8, 2020 •

edited

Loading

kbrock commented Dec 8, 2020 •

edited

Loading

miq-bot commented Dec 8, 2020

kbrock commented Dec 8, 2020

bdunne left a comment

Fryguy commented Dec 8, 2020

Hide deprecated and duplicate attributes #20664

Hide deprecated and duplicate attributes #20664

Conversation

kbrock commented Oct 7, 2020

d-m-u left a comment

Choose a reason for hiding this comment

NickLaMuro left a comment

Choose a reason for hiding this comment

kbrock commented Oct 9, 2020

Fryguy commented Oct 12, 2020

d-m-u commented Oct 15, 2020

NickLaMuro commented Nov 16, 2020 • edited Loading

NickLaMuro left a comment • edited Loading

Choose a reason for hiding this comment

NickLaMuro Nov 16, 2020

Choose a reason for hiding this comment

bdunne left a comment

Choose a reason for hiding this comment

kbrock commented Nov 16, 2020 • edited Loading

kbrock commented Nov 17, 2020

kbrock commented Nov 17, 2020

kbrock commented Nov 17, 2020 • edited Loading

kbrock commented Dec 8, 2020

kbrock commented Dec 8, 2020

Fryguy commented Dec 8, 2020 • edited Loading

kbrock commented Dec 8, 2020 • edited Loading

miq-bot commented Dec 8, 2020

kbrock commented Dec 8, 2020

bdunne left a comment

Choose a reason for hiding this comment

Fryguy commented Dec 8, 2020

NickLaMuro commented Nov 16, 2020 •

edited

Loading

NickLaMuro left a comment •

edited

Loading

kbrock commented Nov 16, 2020 •

edited

Loading

kbrock commented Nov 17, 2020 •

edited

Loading

Fryguy commented Dec 8, 2020 •

edited

Loading

kbrock commented Dec 8, 2020 •

edited

Loading