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.

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

ENT-5581: Update messaging around the "container mode" #3294

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

m-horky
Copy link
Contributor

@m-horky m-horky commented Jul 3, 2023

  • Card ID: ENT-5581

@cnsnyder cnsnyder requested review from a team and cnsnyder and removed request for a team July 3, 2023 09:02
@m-horky m-horky force-pushed the mhorky/ENT-5581_secrets-mode branch from 60a6bf1 to 6478f02 Compare July 3, 2023 09:09
@github-actions
Copy link

github-actions bot commented Jul 3, 2023

Coverage

Coverage (computed on Fedora latest) •
FileStmtsMissCoverMissing
subscription_manager/cli_command
   cli.py2093185%65, 69, 124, 128–129, 144, 201, 205, 207, 250, 289, 300–302, 316–318, 342, 362, 388, 397–398, 402–403, 419, 421–422, 424–425, 430, 438
TOTAL18158456374% 

Tests Skipped Failures Errors Time
2633 9 💤 0 ❌ 0 🔥 1m 3s ⏱️

Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

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

I think that this changes is not necessary. It will only confuse users.

@m-horky m-horky marked this pull request as draft July 3, 2023 12:47
@ptoscano
Copy link
Contributor

The thing is that, after #3270 and #3292, the detection logic for the "container mode" does not depend on running in a container. In fact, the only way the "mode" will be triggered is the presence of secrets (certificates, config) in certain locations, and nothing else. Hence, I'd argue the opposite: I'd find misleading the message "disabled when running in a container" when subscription-manager would run in a container with no secrets provided; strictly speaking, you don't even need to be in a container to trigger that mode, you can theoretically trigger it even on a VM or baremetal system by placing the entitlement certificates in the places for it.

@jirihnidek
Copy link
Contributor

You are correct that you can theoretically mimic this situation on any system, but it is unlikely that users and customers will do that. Why would they do that?

I would not change this message, because there could be some theoretical edge case. I think that changing this message would be only confusing for existing users.

@ptoscano
Copy link
Contributor

The point of changing the message is avoiding the confusion actually:

  • if you start a container and no secrets are provided, subscription-manager will work just fine (as on a baremetal system, or in a VM)
  • if you start a container with secrets provided, subscription-manager still disables itself with "disabled when running in a container"

I find this inconsistent, because it is not clear why the first container does not cause subscription-manager to disable itself, even though it is actually running in a container! Since the factor here is only the presence of secrets, then simply tell that to the user, so they know directly why their subscription-manager does not allow them to register/etc.

@jirihnidek
Copy link
Contributor

The point of changing the message is avoiding the confusion actually:

* if you start a container and no secrets are provided, `subscription-manager` will work just fine (as on a baremetal system, or in a VM)

* if you start a container with secrets provided, `subscription-manager` still disables itself with "disabled when running in a container"

It is fair, but IMHO proposed message could be still confusing... What about extending message like this:

Subscription Manager is operating in container mode (secrets in /etc/pki/entitlement-host detected).

We would be messaging that user tries to use subscription-manager inside UBI container and how was container detected.

I find this inconsistent, because it is not clear why the first container does not cause subscription-manager to disable itself, even though it is actually running in a container! Since the factor here is only the presence of secrets, then simply tell that to the user, so they know directly why their subscription-manager does not allow them to register/etc.

@ptoscano
Copy link
Contributor

The point of changing the message is avoiding the confusion actually:

* if you start a container and no secrets are provided, `subscription-manager` will work just fine (as on a baremetal system, or in a VM)

* if you start a container with secrets provided, `subscription-manager` still disables itself with "disabled when running in a container"

It is fair, but IMHO proposed message could be still confusing... What about extending message like this:

Subscription Manager is operating in container mode (secrets in /etc/pki/entitlement-host detected).

I'd still remove the "container mode", that is the whole point of the changes here. There is not much left of "container" here.

We would be messaging that user tries to use subscription-manager inside UBI container and how was container detected.

As I said, being in a container does not have any influence in how subscription-manager behaves now.

* Card ID: ENT-5581

The two messages that would be printed in containers were not consistent
and slightly contradictory (is operating x is disabled).

This change unifies them and ensures that the tool name is always
'subscription-manager'. It also updates the CLI message which now better
describes the action necessary to manage the subscriptions via
the host system.
@m-horky m-horky force-pushed the mhorky/ENT-5581_secrets-mode branch from 6478f02 to 4dfcbc4 Compare July 20, 2023 07:59
@m-horky m-horky marked this pull request as ready for review July 20, 2023 08:00
@cnsnyder cnsnyder requested a review from a team July 20, 2023 08:00
@m-horky m-horky changed the title ENT-5581: Rename "container mode" to "secrets provided mode" ENT-5581: Update messaging around the "container mode" Jul 20, 2023
@ptoscano ptoscano removed the request for review from a team July 20, 2023 11:28
@ptoscano ptoscano merged commit 6143255 into main Jul 20, 2023
17 checks passed
@ptoscano ptoscano deleted the mhorky/ENT-5581_secrets-mode branch July 20, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants