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
Fixes #33750 - Make katello commands work without Puppet #818
Fixes #33750 - Make katello commands work without Puppet #818
Conversation
Issues: #33750 |
49620de
to
5507063
Compare
Thank you for tackling this! Running the bats tests I see better output but a few failures still:
|
bd7fc8b
to
7f444f1
Compare
@ehelms, sorry for the delay, updated. Could you please test this out for both flows: with Puppet and without Puppet? Or, if you don't have time, please send me your steps how to configure the test env and run the tests for those flows. I've tried what you suggested, but for some reason Katello is not being installed there... |
Getting better! Down to this failure now:
|
7f444f1
to
2747168
Compare
My apologies 🤦 The logic in this plugin is not so straightforward and it's not easy to debug without context and usage knowledges. This needs to be refactored and we need to get rid of One more test round 🚀 |
Tested again and still hitting the issue. What it looks like is happening is: The code does a fetch on environments but passes the name of the activation key as the name parameter rather than passing the name of the lifecycle-environment that was input on the command. The command run is:
|
This is weird, and I have no idea what's going on... I've tested this command and this should fix that. Also, I've tried now with and without Puppet, and it seems to pass everything just fine: |
@ofedoren Let me take it for another test drive, there is a possibility I somehow had stale code so I will do everything fresh. |
This worked for me! I must have had a bad environment the first time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worked for me in testing in the right scenario. @chris1984 could you take a look as a maintainer code and functionality wise? This is blocking merging some turning Puppet off by default work so I would appreciate priority on it.
@ehelms sure will look after standup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK it looks good, the only comment I had was around the legacy_option_id/s
method name, if we could name that something better to know what it used to be part of, but git blame can see this pr and I can't really think of something better at the moment, I don't want to block Eric's stuff. Thank you for adding test coverage back as well.
@ehelms, could you please test this out?