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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Drop Supports' unsupports variable #22898

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Drop Supports' unsupports variable #22898

merged 1 commit into from
Aug 22, 2024

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Feb 14, 2024 •
edited
Loading

Overview

Drop class level Model.unsupported. It is modified by every Model.supports?(:feature) call and has a race condition.

Before

  • Setup Model.supports_features hash once at startup. Not modified again.
  • Determine supports?(:feature) from supports_features hash.
  • Call block if necessary and store the result in Model.unsupported hash every time.
  • This is a non-thread-safe race condition.
  • Read and return value from unsupported

After

  • Setup Model.supports_features hash once at startup. Not modified again.
  • Determine supports?(:feature) from supports_features hash.
  • Call unsupported_reason block if necessary, but don't store results in class level variables.
  • Return the result.

Requirements

Depends upon:

precursor:

@kbrock kbrock requested review from agrare and Fryguy as code owners February 14, 2024 23:41
@kbrock kbrock mentioned this pull request Feb 14, 2024
Merged
23 tasks
@miq-bot miq-bot added the wip label Feb 14, 2024
@kbrock kbrock force-pushed the supports_unsupports branch 2 times, most recently from ae70c63 to 8c5f663 Compare February 23, 2024 02:26
This was referenced Feb 23, 2024
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@kbrock kbrock force-pushed the supports_unsupports branch from 8c5f663 to 4d8dd9e Compare March 1, 2024 01:50
@kbrock
Copy link
Member Author

kbrock commented Mar 1, 2024

update:

  • rebased (fixed unmergable)
  • reduced changes in check_supports

@kbrock kbrock force-pushed the supports_unsupports branch from 4d8dd9e to 9dc6541 Compare March 7, 2024 18:16
@kbrock
Copy link
Member Author

kbrock commented Mar 7, 2024

update:

  • rebase (fix merge conflicts)

@kbrock kbrock removed the unmergeable label Mar 7, 2024
@kbrock kbrock changed the title [WIP] Drop Supports' unsupports variable Drop Supports' unsupports variable Apr 23, 2024
@kbrock
Copy link
Member Author

kbrock commented Apr 23, 2024

up-wip: still waiting on #22976 - just un-WIPing to show this is good to go

@kbrock kbrock force-pushed the supports_unsupports branch from 3ade819 to 6aa38cc Compare April 26, 2024 00:44
@kbrock kbrock force-pushed the supports_unsupports branch from 6aa38cc to f791e24 Compare May 8, 2024 17:03
@kbrock
Copy link
Member Author

kbrock commented May 23, 2024

@Fryguy anything special to push this forward? let me know if you want me to split into 2 PRs (for each commit)

@kbrock kbrock force-pushed the supports_unsupports branch from f791e24 to 065be55 Compare May 24, 2024 17:11
@kbrock
Copy link
Member Author

kbrock commented May 24, 2024

update:

  • rebase

This was referenced Jul 1, 2024
Merged
Merged
@kbrock kbrock force-pushed the supports_unsupports branch from 065be55 to 84675ca Compare July 1, 2024 22:15
This was referenced Jul 1, 2024
Merged
Merged
kbrock added a commit to kbrock/manageiq-providers-ibm_power_vc that referenced this pull request Jul 1, 2024
@kbrock
Remove return from supports block
a314621 
Part of ManageIQ/manageiq#22898 (as a followup)

Introduced by ManageIQ/manageiq-providers-ibm_power_hmc#153

You can not have a return in a block. It causes a LongJump error
Besides, it tries to return from inside the calling block - not what we want.
kbrock added a commit to kbrock/manageiq-providers-ibm_power_vc that referenced this pull request Jul 1, 2024
@kbrock
Remove return from supports block
d34ec11 
Part of ManageIQ/manageiq#22898 (as a followup)

Introduced by ManageIQ#103

You can not have a return in a block. It causes a LongJump error
Besides, it tries to return from inside the calling block - not what we want.
@kbrock kbrock mentioned this pull request Jul 1, 2024
Merged
kbrock added a commit to kbrock/manageiq-providers-ibm_power_vc that referenced this pull request Jul 1, 2024
@kbrock
Remove return from supports block
6f86590 
Part of ManageIQ/manageiq#22898 (as a followup)

Introduced by ManageIQ#103

You can not have a return in a block. It causes a LongJump error
Besides, it tries to return from inside the calling block - not what we want.
kbrock added a commit to kbrock/manageiq-providers-ovirt that referenced this pull request Jul 1, 2024
@kbrock
Remove return from supports block
ac2ee1a 
Part of ManageIQ/manageiq#22898 (as a followup)

Introduced by ManageIQ#659

You can not have a return in a block. It causes a LongJump error
Besides, it tries to return from inside the calling block - not what we want.

Fixes a missed unsupported reason check
@kbrock kbrock mentioned this pull request Jul 1, 2024
Merged
@kbrock kbrock force-pushed the supports_unsupports branch from 84675ca to 8bb9418 Compare July 8, 2024 20:13
@kbrock kbrock force-pushed the supports_unsupports branch from 8bb9418 to b0768b7 Compare July 16, 2024 22:05
@kbrock
Copy link
Member Author

kbrock commented Jul 16, 2024

update:

  • rebased

All repos are all ready for this commit

@kbrock kbrock force-pushed the supports_unsupports branch from b0768b7 to ce9c449 Compare August 10, 2024 01:05
@miq-bot
Copy link
Member

miq-bot commented Aug 10, 2024

Checked commit kbrock@ce9c449 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@kbrock
Copy link
Member Author

kbrock commented Aug 10, 2024

update:

  • rebased

@Fryguy Any ideas how to get this over the line? Probably merged 45+ PRs to get this far. (though only see 25 listed here). +10/-53 - basically just deleting one class level variable.

kbrock
kbrock commented Aug 20, 2024
View reviewed changes
app/models/mixins/supports_feature_mixin.rb
Comment on lines -104 to +94
def check_supports(feature, instance:)
instance.send(:unsupported).delete(feature)

# query the class if the feature is supported or not
def unsupported_reason(feature, instance: self)
# undeclared features are not supported
value = supports_features[feature.to_sym]

if value.respond_to?(:call)
begin
# for class level supports, blocks are not evaluated and assumed to be true
result = instance.instance_eval(&value) unless instance.kind_of?(Class)
# if no errors yet but result was an error message
# then add the error
if !instance.send(:unsupported).key?(feature) && result.kind_of?(String)
instance.send(:unsupported_reason_add, feature, result)
end
result if result.kind_of?(String)
Copy link
Member Author

@kbrock kbrock Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is the crux of this pr.

Before

When asked, clear unsupported and then setting it with new value.

After

return the value and don't use unsupported

@Fryguy Fryguy merged commit 3f7c1b7 into ManageIQ:master Aug 22, 2024
8 checks passed
@kbrock kbrock deleted the supports_unsupports branch August 23, 2024 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy approved these changes

@agrare agrare Awaiting requested review from agrare agrare is a code owner

Assignees

@Fryguy Fryguy

Labels
refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kbrock @miq-bot @Fryguy