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

Remove katello-agent #455

Merged
merged 1 commit into from Sep 12, 2023
Merged

Conversation

wbclark
Copy link
Contributor

@wbclark wbclark commented Aug 15, 2023

No description provided.

@ehelms
Copy link
Member

ehelms commented Aug 16, 2023

Before we merge any of the changes, we will need staged updates for our pipeline tests, https://github.com/theforeman/forklift/blob/master/bats/fb-katello-client.bats#L10 and any other related katello-agent tests will need scoping. The puppet module tests now remove the testing of an enabled qpid being disabled so we should consider either:

  1. Keeping a test here that first enables all the qpid bits and then tests that the module disables it properly
  2. Add a pipeline upgrade test, similar to how we did Pulp 2 removal (https://github.com/theforeman/forklift/blob/master/pipelines/upgrade/07-server_to_final.yml#L24) that ensures katello-agent is enabled before the test upgrades to nightly / 4.10

@evgeni if you have any thoughts on this

@evgeni
Copy link
Member

evgeni commented Aug 22, 2023

I think I'd prefer the forklift/bats route, as that's what users will be facing: having a 4.9 with k-a, upgrading to 4.10 and abandoning k-a.

manifests/init.pp Outdated Show resolved Hide resolved
spec/acceptance/dispatch_router_spec.rb Outdated Show resolved Hide resolved
@@ -1,81 +1,8 @@
# @summary Install and configure Qpid Dispatch Router
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could change this to 'Uninstall Qpid Dispatch Router' but this comment got me thinking that the class could just be removed and what little content remains could be moved into init.pp

@wbclark wbclark force-pushed the remove_katello_agent branch 2 times, most recently from 2c802a0 to e6e73bf Compare August 23, 2023 20:18
@wbclark
Copy link
Contributor Author

wbclark commented Aug 23, 2023

Thanks for the feedback @ekohl

I amended the commit with initial changes based on suggestions

I then added a second commit which further removes the dispatch_router class entirely, moving the little remaining code for cleanup into the base class.

I kept that as a 2nd commit for ease of review, but intend for it to be squashed before merging.

@ehelms
Copy link
Member

ehelms commented Aug 28, 2023

I think this generally looks good, without a test with it setup and then running the module it's harder to tell if we missed something or not. I think in hindsight it would have been simpler to just leave all the code alone but enforce a false on the parameter. That would have reduce the variance being introduced and then allowed ripping all the code out in the following release.

@ekohl please also have a look

@wbclark
Copy link
Contributor Author

wbclark commented Aug 31, 2023

Force pushed to squash commits

@ekohl ekohl merged commit 491a3fc into theforeman:master Sep 12, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants