-
Notifications
You must be signed in to change notification settings - Fork 295
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 #36649 - Remove katello-agent out of legacy UI #10686
Conversation
Issues: #36649 |
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.
Some initial code comments :)
.../javascripts/bastion_katello/content-hosts/bulk/views/content-hosts-bulk-packages-modal.html
Show resolved
Hide resolved
...scripts/bastion_katello/content-hosts/content/content-host-packages-applicable.controller.js
Outdated
Show resolved
Hide resolved
Seems like a few Katello tests need updates. |
@jeremylenz Was it because I changed them from a katello agent task to a rex job? Or is there something else that caused them to fail? |
Looks like the test failures are all around incremental updates.. which I'm not sure is related. We don't know for sure, though, because we don't have the error stack from test.log that would tell us why we got the 500s. Let's run it again and see if it was a fluke. |
[test katello] |
@chris1984 Same three failures. Can you run those tests locally and look at |
@jeremylenz finally green :) Ready for testing and a 2nd review. Once you merge your REX setting removal PR, I will have to rebase this one, but it should be good to test/review at least |
Should be no need, since I plan to merge this one first :) |
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.
Found one straggler:
Line 40 in 05b73f7
$scope.performViaKatelloAgent = function() {}; |
Otherwise LGTM!
24fe5bb
to
9777e05
Compare
@jeremylenz updated and removed the straggler, let me know how this looks |
[test katello] |
Damn vcr failures |
[test katello] |
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 👍
test failures unrelated
Will fill this out when I get the tests green, pushing up to see the mess that I caused. Locally there is like 13 failures.
What are the changes introduced in this pull request?
Considerations taken when implementing this change?
What are the testing steps for this pull request?
Todo: