-
Notifications
You must be signed in to change notification settings - Fork 290
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 #33852 - Content - Errata - Add REX actions #9766
Conversation
Issues: #33852 |
eec9f30
to
1e3b13b
Compare
c41b944
to
12c589a
Compare
@@ -25,6 +25,14 @@ child :content_facet => :content_facet_attributes do | |||
node :katello_tracer_installed do |content_facet| | |||
content_facet.tracer_installed? | |||
end | |||
|
|||
node :katello_agent_enabled do |
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.
I know as a dev it's easy to add these here our sake but it feels a bit weird. Random idea: could the /katello/api/v2 (or similar endpoint) expose some amount of configuration? Multiple places may need this info and a content facet may not be in scope at that point (I don't have an example off the top of my head)?
Or maybe Foreman or other plugin has established something we can use?
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 was discussed also. I could get the configuration separately but having that information embedded in the host details made best sense for me. Given we already have katello_tracer_installed
and katello_agent_installed
there. I dont see why adding katello_agent_enabled
and remote_execution_by_default
attributes should be a red flag. Considering these are details pertinent to this host?
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.
The examples you gave are reflective of the state of the content host - whether tracer or agent is installed. The fields you've added are related to the configuration of Katello which I guess is pertinent but also has nothing to do with the content facet itself. If you and Jeremy are good then let's go with it
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.
I think ForemanContext would be the ideal place to have something like this. It already contains other global app settings. The problem is that adding values there requires Foreman code changes, and also we'd be adding Katello-specific data. Until we can come up with a way to address both of those issues, I think adding it here is... okay but not great. Adding to the / API endpoint is an interesting idea too, but then we'd have to make an additional API call.
I think going with what we have is fine for now, because at least it's an API call that we're already making. Unless someone can think of another brilliant idea :)
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.
First time I've seen ForemanContext and it really does seem like the right place. I'm surprised that component wasn't designed with plugin extensibility in mind. That is something we should look into in the future!
@jturel @jeremylenz Updated this PR and tests to use the |
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.
looking great!
webpack/components/extensions/HostDetails/Tabs/__tests__/errataTab.test.js
Outdated
Show resolved
Hide resolved
Refs #33852 - Addressing bad Template Refs #33852 - Adding tests
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.
Haven't been able to test select all yet but I tested the REX actions and they work as expected.
a few comments below :)
@@ -6,10 +6,10 @@ description_format: 'Install errata %{errata}' | |||
feature: katello_errata_install | |||
provider_type: SSH | |||
template_inputs: | |||
- name: errata | |||
description: A comma separated list of errata to install | |||
- name: Errata Search Query |
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.
- name: Errata Search Query | |
- name: Errata search query |
On all the new pages we've been instructed to use this style of capitalization.
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.
I think this broke the display of the job name. All my tasks now say
Install errata ''
but they should say the errata ID:
Install errata '2021-blahblah-30148u'
I think if you change line 5 above to
description_format: 'Install errata %{Errata search query}'
it will maybe hopefully fix it.
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.
fixed
@@ -19,6 +19,8 @@ foreign_input_sets: | |||
<% advisories = input(:errata).split(',').join(' ') %> | |||
<%= render_template('Package Action - SSH Default', :action => 'install -n -t patch', :package => advisories) %> | |||
<% else %> | |||
<% advisories = input(:errata).split(',').map { |e| "--advisory=#{e}" }.join(' ') %> | |||
<% advisory_ids = @host.advisory_ids(search: input("Errata Search Query")) %> |
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.
<% advisory_ids = @host.advisory_ids(search: input("Errata Search Query")) %> | |
<% advisory_ids = @host.advisory_ids(search: input("Errata search query")) %> |
if (isInstallable) { | ||
rowActions = [ |
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.
For non-installable errata, I think these items should still be there, just disabled.
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.
Its a different list for non installable but applicable.
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.
not sure what you mean here?
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.
For applicable errata you want to show Apply Erratum
which takes em to errata page.
For installable you want to show Apply vi Katello agent
etc.
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.
Ahhh gotcha.
Update webpack/components/extensions/HostDetails/Tabs/__tests__/errataTab.test.js Co-authored-by: Jeremy Lenz <jlenz@redhat.com> Refs #33852 - Addressed comments
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 from code perspective. Looks great and the updates appear to map to the meeting we had last week all while maintaining backward compatibility with the existing REX template. That's awesome!
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.
Thanks for your patience @parthaa
everything seems to work great! Just a couple Katello agent comments
{ errata_ids: [id] }, | ||
)); | ||
|
||
const apply = () => (contentFacet.remoteExecutionByDefault ? applyViaRemoteExecution() : |
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.
The Apply button uses different logic to decide whether to use Katello agent, compared to the dropdown items. This uses
contentFacet.remoteExecutionByDefault
whereas the dropdowns use
contentFacet.katelloAgentInstalled && contentFacet.katelloAgentEnabled
Was this intended? In my environment, the Apply button tries to use Katello agent, but I don't have any Katello agent actions in the dropdowns. This could be confusing.
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.
Changed this condition a little. (contentFacet.remoteExecutionByDefault || !katelloAgentAvailable)
So if the thing cant be installed via katello agent it will still try rex (even if you have rexByDefault false).
Co-authored-by: Jeremy Lenz <jlenz@redhat.com> Update webpack/components/extensions/HostDetails/Tabs/ErrataTab.js Co-authored-by: Jeremy Lenz <jlenz@redhat.com> Update app/views/foreman/job_templates/install_errata.erb Co-authored-by: Jeremy Lenz <jlenz@redhat.com> more
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.
Changes LGTM
APJ 👍
[test katello] |
1 similar comment
[test katello] |
* Fixes #33852 - Content - Errata - Add REX actions (cherry picked from commit 0e7f14c)
* Fixes #33852 - Content - Errata - Add REX actions (cherry picked from commit 0e7f14c)
What are the changes introduced in this pull request?
On the new host errata page
Select All
and excludesApply Erratum
page when you click on the kebab.What are the testing steps for this pull request?
On the dev env
Install Errata - Katello SSH Default
template using the Actions DropdownInstall Errata - Katello SSH Default
templateInputs
tab. You should see 3 inputs with nameFilte Errata
,Inclusion Type
, anderrata
Set up the host
These steps will give you both
Applicable
andInstallable
errata on the host.yum install armadillo-0.1-1.noarch
rpm -ivh https://jlsherrill.fedorapeople.org/fake-repos/needed-errata/walrus-0.71-1.noarch.rpm https://jlsherrill.fedorapeople.org/fake-repos/needed-errata/whale-0.2-1.noarch.rpm https://jlsherrill.fedorapeople.org/fake-repos/needed-errata/shark-0.1-1.noarch.rpm https://jlsherrill.fedorapeople.org/fake-repos/needed-errata/stork-0.12-2.noarch.rpm
Now there are 4 bulk apply operations available on this page
There are also 3 apply operations available on each installable errata
On Applicable errata we should see an
Apply Erratum
link that should take you toErrata - Apply to Content Hosts
page.Select All Errata
Select
Apply by Customized Remote Execution
You should be redirected to the Job Invocations errata install page, where you shoud be able to see elements of the new template
Notice the
Inclusion Type
entry sayingInclude all errata EXCEPT below
. The select all resolves to thisOn same page
Filter Errata
will get populated if you have search query set and done aSelect All
Try out different errata operations and make sure they get applied correctly on the host.
Guidelines for Code Review
Backend Changes
BulkItemsHelper
class so that Errata/Traces can make use of theselect all
.install_errata.erb
template. Added inputs to handle bulk selection operations.advisory_helpers
method in host managed extensions to aid with the new template.Front End Changes