feat: Added levels of connectivity to connect command #179
Conversation
|
Open question:
{
"hostname": "localhost",
"uid": 0,
"enabled_features": [
"analytics",
"remote-management"
],
"disabled_features": [
"content"
],
"rhsm_connected": true,
"content_enabled": false,
"insights_connected": true,
"yggdrasil_started": true
}Another possible solution could be following: {
"hostname": "localhost",
"uid": 0,
"features": {
"enabled": [
"analytics",
"remote-management"
],
"disabled": [
"content"
],
},
"rhsm_connected": true,
"content_enabled": false,
"insights_connected": true,
"yggdrasil_started": true
}Or we can introduce something like this. This has potential to add there more attributes in the future: {
"hostname": "localhost",
"uid": 0,
"features": {
"content": {
"enabled": false,
},
"analytics": {
"enabled": true,
},
"remote-management": {
"enabled": true,
},
},
"rhsm_connected": true,
"content_enabled": false,
"insights_connected": true,
"yggdrasil_started": true
}TODO:
|
There was a problem hiding this comment.
The patch seems solid. I won't comment much on the code itself; we'll likely see further changes done in near future as we stabilize it before new features come.
The output seems to jump around, were the two spaces intentional?
$ rhc connect --disable-feature analytics
...
[✓] Connected to Red Hat Subscription Management
[✓] Content ... Red Hat repository file generated
[ ] Analytics ... Connecting to Red Hat Insights disabled
[✓] Remote Management ... Activated the yggdrasil service
When disconnecting, Insights reported status 1 even though we should have detected we're (already) unregistered:
[✓] Deactivated the yggdrasil service
[𐄂] Cannot disconnect from Red Hat Insights: exit status 1
[✓] Disconnected from Red Hat Subscription Management
The same goes for disconnection: with nothing connected in the first place, the output doesn't say yggdrasil wasn't enabled in the first place. That's a non-blocking note, but it feels strange.
$ rhc connect --enable-feature remote-management --disable-feature remote-management
cannot enable feature: "remote-management": feature "remote-management" explicitly disabled
Are we able to display something prettier and more human-understandable? Non-blocking note, may be addressed later.
| if uiSettings.isMachineReadable { | ||
| connectResult.EnabledFeatures = append(connectResult.EnabledFeatures, feature.ID) | ||
| } | ||
| featuresStr = append(featuresStr, "["+symbolOK+"]"+feature.ID) |
There was a problem hiding this comment.
I think there should be a space between the checkbox and the text.
There was a problem hiding this comment.
I would like to have some white space between checkbox and the text too, but standard "space" is IMHO too long and final line could be confusing at the first glance:
Current behavior:
Features preferences: [✓]content, [ ]analytics, [ ]remote-management
Proposed behavior:
Features preferences: [✓] content, [ ] analytics, [ ] remote-management
There was a problem hiding this comment.
That looks fine to me, I wouldn't worry too much.
| if uiSettings.isMachineReadable { | ||
| connectResult.DisabledFeatures = append(connectResult.EnabledFeatures, feature.ID) | ||
| } | ||
| featuresStr = append(featuresStr, "[ ]"+feature.ID) |
There was a problem hiding this comment.
I think there should be a space between the checkbox and the text.
There was a problem hiding this comment.
Same comment here.
I like the nested approach. Would it make sense to move the "features": {
"content": {
"enabled": true,
"connected": true,
},
"analytics": {
"enabled": true,
"connected": false,
},
...
} |
rhsm.go
Outdated
| if err := privConn.Object( | ||
| "com.redhat.RHSM1", | ||
| "/com/redhat/RHSM1/Register").Call( | ||
| "com.redhat.RHSM1.Register.RegisterWithActivationKeys", | ||
| dbus.Flags(0), | ||
| orgID, | ||
| activationKeys, | ||
| map[string]string{}, | ||
| map[string]string{"enable_content": enabledContentStr}, |
There was a problem hiding this comment.
When I try to register with my activation key, I get the following error:
[link@lima-rhel-10 rhc]$ sudo ./builddir/rhc connect -o 12671438 -a developer
Connecting lima-rhel-10 to Red Hat.
This might take a few seconds.
Features preferences: [✓]content, [✓]analytics, [✓]remote-management
[𐄂] Cannot connect to Red Hat Subscription Management
[𐄂] Skipping generation of Red Hat repository file
[𐄂] Skipping connection to Red Hat Insights
[𐄂] Skipping activation of yggdrasil service
Successfully connected to Red Hat!
Manage your connected systems: https://red.ht/connector
The following errors were encountered during connect:
TYPE STEP ERROR
ERROR rhsm cannot connect to Red Hat Subscription Management: error: Unknown arguments: dict_keys(['enable_content'])
Does the D-Bus method RegisterWithActivationKeys not accept 'enable_content' as
a parameter option? If so, does this mean that we cannot opt out of content if
registering with an activation key?
There was a problem hiding this comment.
@subpop Good point. I am working on the fix fix for rhsm.service. It should be simple.
There was a problem hiding this comment.
The PR for subscription-manager created: candlepin/subscription-manager#3488
I like this as well. Putting both "connected" and "enabled" inside the JSON object seems like the most scalable approach. |
|
@m-horky @subpop OK, it seems we have consensus that we will use the third variant of proposed JSON format. BTW: We can do big changes of JSON format ATM, because it hasn't been shipped in any official release. We can drop {
"hostname": "localhost",
"uid": 0,
"rhsm_connected": true,
"features": {
"content": {
"enabled": false,
"successful": true,
},
"analytics": {
"enabled": true,
"successful": false,
},
"remote_management": {
"enabled": true,
"successful": true,
},
},
} |
jirihnidek
force-pushed
the
jhnidek/connect_levels_of_connectivity
branch
from
170304e to
bd93706
Compare
|
Hi, this is my first attempt to get hands on for this feature. When I am trying to enable only one feature I still see everything getting enabled, for example I am trying to enable only content but all three feature are enabled. This should not happen, I should not need to mention disable-feature explicitly- |
The intention of In theory we can change behavior of |
|
@subpop @jirihnidek I have one more concern, when rhc connect is run without --enable-feature or --disable-feature then by default connectivity level should be “level 2: analytics” (Did this requirement change overtime, not sure so I am just highlighting ) ` |
I don't like the idea of @Archana-PandeyM In the current design, to accomplish what you were trying to do, you would need to disable the features you don't want: |
rhc today enables all three levels by default, so the design here maintains that. I'll check with Christian if he has any expectation around changing the default enabled features. |
|
@jirihnidek With current implementation if 'Analytics' is disabled then 'remote-management' should be disabled too. but looks like there is issue-: In the command output above 'analytics' is disabled but 'remote-management' is enabled. We should throw error instead or disable both. |
jirihnidek
force-pushed
the
jhnidek/connect_levels_of_connectivity
branch
2 times, most recently
from
dfbefe4 to
eda17d4
Compare
|
@Archana-PandeyM you are right. I forgot to consider this case. I altered behavior a little bit, when |
There was a problem hiding this comment.
One tiny consideration about the location of smallIndent and mediumIndent. Otherwise, this looks good to me from a code perspective.
I tested the behavior too and it works as expected with username and password. Activation keys still don't work due to candlepin/subscription-manager#3488.
|
@jirihnidek One last observation for this round of testing, I noticed that even though I requested to disable 'content' and the rhc connect says the file was not generated, Still redhat.repo file has repository details. Is the change in subman build(Do I need to install a seperate build)? Here are the reproducer steps: |
|
@Archana-PandeyM yeah, you also need to use build of subscription-manager from this PR: candlepin/subscription-manager#3488 (do not forget to restart rhsm.service) Otherwise, the content is not disabled due to bug in subscription-manager (nonempty value including |
* Card ID: CCT-1005
* First step of levels of connectivity
* When connect command is used, then it is possible to
enabled or disable three features using command line
options --enable-feature or --disable-feature. Following
features are supported
- content
- analytics
- remote-management
* Added checks for allowed combination of CLI options
* Modified UI little bit to be able to visualize that
feature was not started
* Output to JSON format is still supported, but it was
modified.
* When machine readable format is used, then information
about features could be following:
{
"hostname": "localhost",
"uid": 0,
"rhsm_connected": true,
"features": {
"content": {
"enabled": true,
"successful": true
},
"analytics": {
"enabled": true,
"successful": false,
"error": "some error message"
},
"remote_management": {
"enabled": false,
"successful": false
}
}
}
jirihnidek
force-pushed
the
jhnidek/connect_levels_of_connectivity
branch
from
eda17d4 to
73d3974
Compare
|
@jirihnidek Thanks! |
|
OK, I have result of investigation. It seems that When D-Bus methods If we really want to disable content, then we probably have to disable managing content in |
|
User can run I think that |
|
The |
Agreed. |
This makes sense. The first run of The original design intent of |
|
@Archana-PandeyM @subpop the issue of not disabled content is fixed in the PR for subscription-manager: candlepin/subscription-manager#3500 |
The current behavior of This is the reason, why Question is: Shouldn't we also introduce |
No. Introducing a If it is an error to run "register/connect" on a system that is already registered/connected, then the Now, that means that we probably do need to make changes to this PR so that |
enabled or disable three features using command line
options
--enable-featureor--disable-feature. Followingfeatures are supported
contentanalyticsremote-managementfeature was not started
about enabled/disabled content to generated JSON file
enabled/disable features is added to the generated JSON doc
disconnectcommand to looks similar toconnectcommand