fix: Remove Auto-attach button #68
Conversation
jirihnidek
commented
Jun 5, 2024
•
jirihnidek
commented
- Card ID: CCT-425
- Removed "Auto-Attach" button, because corresponding D-Bus method was removed from rhsm.service due to SCA only effort
- Renamed "Auto-attach" option from registration dialog to "Enable content"
- Modified integration tests
* Card ID: CCT-425 * Removed "Auto-Attach" button, because corresponding D-Bus method was removed from rhsm.service due to SCA only effort
|
Please adapt the tests as well. |
* Card ID: CCT-425 * Keyword auto-attach was replaced with name enable content, because auto-attach does not make sense in SCA mode and enable content is more decent
* Card ID: CCT-425 * Modify integration tests according changes of cockpit. Replacing keyword auto-attach with enable content and removing button "Auto-Attach"
|
I would like to remove some attach bits from cockpit... |
* Card ID: CCT-425 * Removed unused attach service * Rephrased one error message
Hmm do we still need this option? What are the cases in which you don't want to enable the content automatically? Personally speaking, I'd be fine in dropping the "Attach automatically" checkbox and unconditionally enable the content. This would be the same as what |
I'm not against. I like deleting code that could only confuse users and cause problems... |
jirihnidek
force-pushed
the
jhnidek/remove_auto_attach
branch
from
008cb70
to
47865fe
Compare
* Card ID: CCT-425 * Removed an option to not enable content on registration dialog * Removed related integration test
jirihnidek
force-pushed
the
jhnidek/remove_auto_attach
branch
from
47865fe
to
0ca1321
Compare
|
@ptoscano checkbox removed. |
There was a problem hiding this comment.
Mostly OK; there is some code unrelated to auto-attaching that was accidentally dropped.
Also, please rebase this against main, and squash the commits into one with the proper commit message for all the changes (all the intermediate changes are not needed).
Thanks!
| let insights_checkbox_disabled = true; | ||
| if (this.props.insights_available === true) { | ||
| insights_checkbox_disabled = false; | ||
| } else { | ||
| if (this.props.auto_attach === true) { | ||
| insights_checkbox_disabled = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Part of this deleted code is still needed though: in particular, the insights checkbox must be disabled when insights-client is not available; the only needed part that can be removed is the else {...} block.
There was a problem hiding this comment.
When insights-client is not available, then it will be automatically installed. When content is always enabled, then it could not happen that it will not be possible to install insights-client during registration process. Thus, this change is correct.
There was a problem hiding this comment.
When insights-client is not available, then it will be automatically installed.
... if it's available in any repository; the code takes that potential (failing) situation into account already, so that should be considered here. See the detectInsights function in src/subscriptions-client.js.
There was a problem hiding this comment.
You are wrong. Function detectInsights tries to detect following things. First, it checks if insights-client command is possible to execute. If insights-client command does not exists, then it checks if PackageKit is available. It really does not check if insights-client RPM is available in any repository.
There was a problem hiding this comment.
My original point still stands: the code there disables the possibility to register to Insights if insights-client is not installed, and PackageKit is available. While it is a corner case, it needs to still be kept into account.
There was a problem hiding this comment.
Oh, PackageKit could be removed... There is no RPM cockpit package requiring this PackageKit RPM. Interesting.
There was a problem hiding this comment.
Removed, or even only disabled by the user. That's why I said "available" previously: "not installed" and "installed and disabled" are basically the same, from our POV.
| insights = [ | ||
| <FormGroup key="0" fieldId="subscription-insights" label={_("Insights")} hasNoPaddingTop> | ||
| <Checkbox id="subscription-insights" isChecked={this.props.insights} | ||
| label={ Insights.arrfmt(_("Connect this system to $0."), Insights.link) } | ||
| isDisabled={ insights_checkbox_disabled } |
There was a problem hiding this comment.
this is still needed (see above)
There was a problem hiding this comment.
No, it is not. See my response above.
