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

chore: Remove artifacts of attach #3476

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Glutexo
Copy link
Contributor

@Glutexo Glutexo commented Nov 27, 2024

Removed what remained after removing the attach command. A system is now attached upon registration.

The command was removed in #3409 for CCT-425.

Card IDs:

@Glutexo
Copy link
Contributor Author

Glutexo commented Nov 27, 2024

In this commit, I removed everything related to the attach CLI command. It doesn’t deal with the --auto-attach option of the register command. I know only little about Subscription Manager, so I did so mostly blindly, only after a short conversation with Matyáš. @pinotree, may I ask you to take a look?

@Glutexo
Copy link
Contributor Author

Glutexo commented Nov 27, 2024

I created this pull request as a draft, thinking that I’d put in the command removals as individual commits. If @pinotree approves, however, I think we can merge and do the other commands in (a) new pull request(s).

@ptoscano
Copy link
Contributor

Removed what remained after removing the attach CLI command.

strictly speaking, this also removes other references related to "attach" (eg dbus, in some old/unused tests, etc), not only in the CLI

@Glutexo Glutexo marked this pull request as ready for review November 28, 2024 14:26
@Glutexo Glutexo force-pushed the remove-unused branch 2 times, most recently from 307afaf to 43e8958 Compare November 28, 2024 14:28
@Glutexo
Copy link
Contributor Author

Glutexo commented Nov 28, 2024

@ptoscano Thank you for the review! 🙇🏻‍♂️ I addressed your comments.

I un-drafted the PR, so it can be merged if you approve of the changes. There are already 10 changed files. That’s enough for a single PR.

Removed what remained after removing the `attach` command. A system is now attached upon registration.

Card IDs:
* CCT-603

Signed-off-by: Štěpán Tomsa <[email protected]>
Removed what remained after removing the `--auto-attach` option of `register`. A system is now always attached upon registration.

Card IDs:
* CCT-603

Signed-off-by: Štěpán Tomsa <[email protected]>
@Glutexo
Copy link
Contributor Author

Glutexo commented Dec 3, 2024

After a discussion with Barbora and Matyáš, we agreed to rather put the command removals into this pull request as individual commits:

Removed what remained after removing the `autoheal` option from Action clients (ENT-5549).

Card IDs:
* CCT-603

Signed-off-by: Štěpán Tomsa <[email protected]>
@Glutexo
Copy link
Contributor Author

Glutexo commented Dec 3, 2024

Matyáš suggested we can be more aggressive removing code that seems unused. The reasoning was that either integration tests would fail, or QE would find an issue when doing manual testing.

Under that suggestion, I removed quite some methods from EntitlementService and EntitlementDBusObject that doesn’t seem to be called or referenced from anywhere. Added a commit 8c3400b.

Removed what remained after removing the `remove` command.

Card IDs:
* CCT-603

Signed-off-by: Štěpán Tomsa <[email protected]>
Removed what remained after removing the `redeem` command.

Card IDs:
* CCT-603

Signed-off-by: Štěpán Tomsa <[email protected]>
@Glutexo
Copy link
Contributor Author

Glutexo commented Dec 3, 2024

Added fda942a removing artifacts of the redeem command.

Removed what remained after removing the `import` command.

Card IDs:
* CCT-603

Signed-off-by: Štěpán Tomsa <[email protected]>
@Glutexo
Copy link
Contributor Author

Glutexo commented Dec 3, 2024

Added 516d70b removing artifacts of the import command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants