-
Notifications
You must be signed in to change notification settings - Fork 286
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 #24428,#24424 - UI Modularity streams - Host #7724
Conversation
8ae3614
to
152c080
Compare
152c080
to
ed27bc5
Compare
Api Changes
UI Changes
Setup: Going to have setup Remote execution on the dev environment. Rough instructions if yours is not very old
Importing Job templates:
On your consumer
Alternatively you can also artificially add an entry in the db to ContentFacetRepository via rails console to |
@parthaa I'll test functionality further, but a couple questions: Why not return the actual id and create a new field for the name + stream i.e.
(that name was the best I can come up with right now) What about this for the action menu? (cc @Rohoover) initial menu looks like this:
Then that opens up a custom modal: |
Remember name and stream are duplicated across many versions. The query generating this uses a group by / distinct. If you argue get the "latest" or the "first" id, I 'd then have to do a secondary query for each of these guys. This output is useful for the host module streams page and what I have there is sufficient. |
Let's do it. |
@Rohoover @johnpmitsch @akofink just letting y'all know that we don't do this in package and errata install pages, but I m open to it, |
@parthaa ok, that makes sense. I think Can you check if the |
ed27bc5
to
0737619
Compare
I called it module_spec
|
|
Out of curiosity, does searching by module_name or module_stream do anything? As a general statement, we have an issue in the UI whereas the syntax for searching doesn't match the column headers. Many users have pointed this out to me. |
@Rohoover These are the current fields you can search by, they match the column names in this case. (the column covered up is name) What @parthaa is adding is a way to search by dnf's syntax, which they call 'module_spec' in their docs, this isn't a column since we don't store that info and create it from the name and stream. |
Great. Thanks! |
0737619
to
fcd7c83
Compare
@johnpmitsch @Rohoover @akofink @jlsherrill @omkarkhatavkar updated the actions part |
fcd7c83
to
9c65d9e
Compare
This comment has been minimized.
This comment has been minimized.
a9dd373
to
2943a27
Compare
2943a27
to
0f9b279
Compare
When looking at this table, how would you know whether something is unlocked in order to lock it or vice versa? Can we add in the lock icon column to this table or is that info displayed somewhere else. I'd argue the same for other functions. Perhaps I'm missing something... |
What 'is' a locked module? If it is something that cannot be changed, and we do not know it's current status, how do we report to a user when they try to complete an action on a locked item? How would they know to unlock 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 like the UI and workflow a lot! 💯
Each action works well and I'm able to see the changes on my host. Code looks good as well.
One open question about the api docs, let me know if you see the same thing. Since you are using apipie according to the docs I don't think its an error in this change.
We aren't able to currently get the locked/unlocked status from the host until we get changes from subscription manager. Currently, Katello has no idea about the current status of the modules on the host, it can only send actions to the host. Once we have the sub manager changes, we will add the locked/unlocked status as you have specified in the mockups. (partha correct me if any of this is wrong) |
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.
missed one thing, sorry to take back the ACK! Can you add tests for the new API params in test/controllers/api/v2/module_streams_controller_test.rb
?
27995d3
to
02992cc
Compare
added |
[test katello] |
02992cc
to
c21bd1d
Compare
@@ -40,6 +40,11 @@ def inputs | |||
{ :errata => params[:name] } | |||
elsif feature_name == 'katello_service_restart' | |||
{ :helper => params[:name] } | |||
elsif feature_name =~ /module_stream/ | |||
fail HttpErrors::NotFound, _('module streams not found') if params[:module_spec].blank? | |||
inputs = { :module_spec => params[:module_spec] } |
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.
Could we look into passing in :action
too? I think @iNecas originally did this for package actions, where there's a separate template for each thing - install, update, etc, but I feel like the number of templates is getting out of control. I don't quite understand why Katello can't use a single job template for all the features
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 don't recall any significant reason to chose one over the other. Should be doable.
c21bd1d
to
9f2cd4e
Compare
@stbenjam updated |
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.
From a code-perspective it looks good, just a typo I think. I haven't tested it yet
lib/katello/plugin.rb
Outdated
@@ -277,6 +277,9 @@ | |||
RemoteExecutionFeature.register(:katello_group_remove, N_("Katello: Remove Package Group"), :description => N_("Remove package group via Katello interface"), :provided_inputs => ['package']) | |||
RemoteExecutionFeature.register(:katello_errata_install, N_("Katello: Install Errata"), :description => N_("Install errata via Katello interface"), :provided_inputs => ['errata']) | |||
RemoteExecutionFeature.register(:katello_service_restart, N_("Katello: Service Restart"), :description => N_("Restart Services via Katello interface"), :provided_inputs => ['helpers']) | |||
RemoteExecutionFeature.register(:katello_module_stream_action, N_("Katello: Module Stream Actions"), | |||
:description => N_("Perform a Imodule stream action via Katello interface"), |
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.
Typo here? a Imodule
This commit contains the UI functionality to list and enable modularity streams. It displays the list of available streams and provides remote action templates to the user to enable/disable, install/remove and lock/unlock streams.
9f2cd4e
to
4112fdd
Compare
[test-katello] |
[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.
works well 👍 I'm still wondering about the api docs, but I don't think that is related to your changes. Other than that everything looks good.
@parthaa 👍 Looks Great! Some Questions/Issues:
Still in-progress with more testing. |
|
it treats an invalid value as off. Thats the case with any boolean params in katello/foreman api. Feel free to file a foreman issue if you want that to raise error |
This was the Issue No. 2, @parthaa if you want we can make a part of forman issue and work again later. |
@omkarkhatavkar can also reproduce this via
its the name_stream_only causing the error. Going by the ugly sql error.
|
For Issue No. 2 https://projects.theforeman.org/issues/25116 is created, not a blocker issue |
@omkarkhatavkar thanks |
[test katello] |
1 similar comment
[test katello] |
This commit contains the UI functionality to list and enable modularity
streams. It displays the list of available streams and provides remote
action templates to the user to enable/disable, install/remove and
lock/unlock streams.