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

Fixes #19135 - Possibility to limit fields that are displayed #276

Merged

Conversation

@ofedoren
Copy link
Member

commented Jun 8, 2018

Requires #275 to be merged first. PR already goes with that commit for testing.

The issue suggests --columns option while I suggest --only, because it works with all the adapters (not for table nor list commands only) and it's shorter :)

Also it accepts labels, which user sees, as arguments, so as mentioned in the issue e.g.:
hammer subnet list --columns name,vlanid will not work, while
hammer --only name,"vlan id" subnet list will work.

Copy link
Member

left a comment

Very nice start @ofedoren. This is complex task. Thank you for dealing with it together with the other related issues.

My biggest concerns are placing the option on the top level and deleting the fields from commands. Instead we should filter them out just for print.

Please bear in mind that in the end this feature will be used together with default verbosity. That means it can be used also for specifying which additional fields should be displayed. Therefore I'd vote rather calling the option --fields. --only doesn't sound right when you want to use it for adding fields.

I was thinking if we could refactor field filters that we currently use only for removing :id fields to a generic mechanism. We would then pass array of field filters to the output adapter at construction time. That would decouple the functionality.

@@ -39,6 +39,9 @@ class MainCommand < AbstractCommand
option ["--interactive"], "INTERACTIVE", _("Explicitly turn interactive mode on/off"),
:format => HammerCLI::Options::Normalizers::Bool.new,
:context_target => :interactive
option ["--only"], "FIELDS", _("Show selected columns or fields only"),

This comment has been minimized.

Copy link
@tstrachota

tstrachota Jun 18, 2018

Member

Placing the option in the main command isn't very intuitive. Could you move it to the base info and list commands? It also doesn't make sense to use it for create, update and delete.

return unless @context.key?(:only)
return if adapter.class <= HammerCLI::Output::Adapter::TreeStructure
labels = @context[:only].map(&:downcase)
definition.select_fields!(deep_select: true, labels: labels)

This comment has been minimized.

Copy link
@tstrachota

tstrachota Jun 18, 2018

Member

Using select_fields! will remove the fields from the command class for good. This will disallow using the option repeatedly in shell mode:

hammer shell
> host list --fields Id,Name
... will list the hosts
> host list --fields Id,Name,IP
... will break, because the field "IP" no longer exists

Note that at the moment it's not possible to use the option in shell mode at all (see my previous comment).

@@ -69,5 +71,11 @@ def init_adapter(adapter_name)
@adapter ||= self.class.adapters[adapter_name].new(context, self.class.formatters)
end

def adjust_definition(definition)
return unless @context.key?(:only)
return if adapter.class <= HammerCLI::Output::Adapter::TreeStructure

This comment has been minimized.

Copy link
@tstrachota

tstrachota Jun 18, 2018

Member

Ideally we should deal with filtering options at one place only.

@mbacovsky

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

@ofedoren, @tstrachota I updated the issue with outcome of our discussion and some Qs. Feel free to comment there.

@ofedoren ofedoren force-pushed the ofedoren:feat-19135-manage-displayed-fields branch 2 times, most recently from 19197e2 to fe5a962 Aug 28, 2018
@ofedoren

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2018

updated.

Field sets are defined as new output definitions for commands. Field sets could be defined with output(nil || output_definition, 'SET_NAME') do ... end. Field set 'ALL' is a special one which is defined by default as one field set created from all currently available in Command.field_sets. All the fields currently defined with output do ... end go to the 'DEFAULT' field set.

for testing wth HammerCLIForeman:

Now it requires something like:

use_option :fields

output(nil, 'THIN') do
  field :id, _('Id')
  field :name, _('Name')
end

in commands such as HammerCLIForeman::ListCommand and HammerCLIForeman::InfoCommand to get the feature work. Also to enable 'thin' request we need to include
params_pruned['thin'] = true if current_output_definition.thin?
in request_params

To print nested only fields use / delimiter. E.g. to see host's mac only:
hammer host info --id 1 --fields network/mac

I've prepared a PR with those changes for HammerCLIForeman. If there is a need, I can send it.

@ofedoren ofedoren force-pushed the ofedoren:feat-19135-manage-displayed-fields branch 2 times, most recently from 5f38160 to db24a68 Aug 31, 2018
@tstrachota

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

Thank you @ofedoren. I'm a bit concerned about the design. Doesn't adding multiple output definitions go against your PR for output modification? #275

Wouldn't it be easier if each field would have notion about which field sets it should be printed in? It should then be just matter of filtering the fields before they're printed and still only one output definition would be used.

@ofedoren ofedoren force-pushed the ofedoren:feat-19135-manage-displayed-fields branch 2 times, most recently from 1884b36 to 7e1ee31 Oct 3, 2018
@ofedoren

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2018

@tstrachota, you're right, it was unnecessary complicated, so I redesignied according to your suggestions:

  • Field sets are specified in options: field :key, :label, :type, sets: ['SETNAME']
  • Only one output definition is used
  • Fields filter does the whole work

Updated docs

Still requires params_pruned['thin'] = true if output.adapter.thin_context?
in request_params for thin request and use_option :fields in ListCommands and InfoCommands

Also, I'm afraid we need to specify 'THIN' set explicitly for Id and Name fields. I was thinking about adding Id and Name fields to THIN set by default from core just looking if field name is id or name, but I don't think it's a good idea.

@ofedoren ofedoren force-pushed the ofedoren:feat-19135-manage-displayed-fields branch from 7e1ee31 to 04933d6 Oct 10, 2018
@tstrachota

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

This needs a rebase.

@ofedoren ofedoren force-pushed the ofedoren:feat-19135-manage-displayed-fields branch from 04933d6 to c1df405 Dec 5, 2018
@ofedoren

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2018

@tstrachota, rebased.

@ofedoren ofedoren force-pushed the ofedoren:feat-19135-manage-displayed-fields branch from c1df405 to 02fa129 Dec 20, 2018

Example option usage could go like this:
```ruby
class HelloCommand < HammerCLI::AbstractCommand
use_option :option_name, :or_names

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Jan 7, 2019

Member

@ofedoren Here I'd expect naming conflict with the option as both could be called --name, also from the example output it is not apparent what it does. There is just one --name option in the output. Maybe it would be better not to mention use_option here and create separate subsection ####Predefined options next to #### Deprecated options. May make sense to explain how it works, how to use it and list available predefined options. With :fields we can add brief description with reference to Printing hash records section for more details.

@@ -41,10 +42,19 @@ def print_collection(fields, collection)
raise NotImplementedError
end

def thin_context?
return false if @context[:fields].nil?

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Jan 7, 2019

Member

Wouldn't @context[:fields] ==['THIN'] do the same as the two lines here?

def render_fields(fields, data)
fields = field_filter.filter(fields)
fields = displayable_fields(fields, data)
fields = filter_fields(fields).filter_by_classes

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Jan 7, 2019

Member

Calling the filters repeats in all the adapters, could it be DRY'ed out a bit?

@@ -25,13 +25,15 @@ def print_error(msg, details=nil, msg_params = {}, options = {})

def print_record(definition, record)
adapter.print_record(definition.fields, record) if appropriate_verbosity?(:record)
adapter.reset_context

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Jan 7, 2019

Member

Why it is necessary to remove the :fields from context?

This comment has been minimized.

Copy link
@ofedoren

ofedoren Feb 7, 2019

Author Member

I do it to be able to reset fields selection in case of using hammer in shell mode.

@mbacovsky

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

@ofedoren, well done!. I like how it works and that the logic is kept quite separated in the adapter. I've left a few comments inline. A few more here:

  • maybe it is time to uncover hammer-cli-foreman part, feel free to mark it [WIP] if not ready
  • regarding THIN and request_params - did you tried to handle it in an option source? It could be possible to avoid changes in request_params
  • regarding THIN and marking params explicitly - did you consider using SEARCHABLES to distinguish the right options? I'm not sure how the thin fields are defined in API and if they match to what we have in the searchables. Also we should have a plan for hammer-cli-katello
  • currently it is not visible for a user what fields are in which sets. I'd love to see some help extension when --fields is defined on the command showing e.g. table like:
Options:
  --fileds    FIELDS  Show specified fileds or predefined filed sets only. (See below)
 
...

Predefined field sets:
+--------+-----+---------+------+
| Fields | ALL | DEFAULT | THIN |
+--------+-----+---------+------+
| Id     |  x  |    x    |  x   |
| Name   |  x  |    x    |  x   |
| Param1 |  x  |    x    |      |
| Param2 |  x  |         |      |
+--------+-----+---------+------+
Copy link
Member

left a comment

@ofedoren see my comments above. The item with help extension may be a bigger addition so either may be replaced with slimmer solution or implemented in separate PR. Its up to your preferences. Do you have any plans on how to let users know what sets are available?

@ofedoren ofedoren force-pushed the ofedoren:feat-19135-manage-displayed-fields branch from 02fa129 to ce55a3a Feb 7, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

@mbacovsky, updated. See inline comments, also:

Updated docs

As you suggested, I use now option sources instead of changing request_params directly, see theforeman/hammer-cli-foreman#407

But still there is a problem: I'm afraid we need to specify 'THIN' set explicitly for Id and Name fields in all the command output definitions (in plugins as well).

About showing help with sets that are available: I like what you suggested, but also I'm afraid if there is a lot of fileds (like in host commands) then the help will be quite big. Would it make sense then to show it in verbose mode (or higher verbosity level)? Also I'll make an issue about showing sets in help after it's merged and resolve it with another PR.

@ofedoren ofedoren force-pushed the ofedoren:feat-19135-manage-displayed-fields branch from ce55a3a to 5659113 Apr 9, 2019
@mbacovsky

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

@ofedoren I'm okay with adding the help extension in separate PR. Otherwise it looks good now.
For the THIN issue - I'd really like to avoid setting it manually. I have an idea:
in the command we have self.output method for output definition. We could extend it in foreman commands with something like:

output_definition.update_field_set('THIN', searchables.for(resource) + ['id])

update_field_set would iterate over the fields in definition and update the set on the matching fields.
Would it make sense?

@ofedoren ofedoren force-pushed the ofedoren:feat-19135-manage-displayed-fields branch from 5659113 to 3ccd557 May 21, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

@mbacovsky, updated. I've added the update_field_set method as you suggested (docs).

The usage of it see: theforeman/hammer-cli-foreman#407

return true if labels.include?(label)

labels.each do |l|
return true if l.start_with?("#{label}/") || label.start_with?(l)

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Jun 3, 2019

Member

@ofedoren finally I was able to figure out how this is supposed to work and fixed the condition to autocomplete with asterisk l.start_with?("#{label}/") || label.match(%r{^#{l.gsub(/\*/, '.*')}(|\/.*)$}). I simplified it a bit also and have

   def include_by_label?(labels, label)
      labels.any? do |l|
        l.start_with?("#{label}/") || label.match(%r{^#{l.gsub(/\*/, '.*')}(|\/.*)$})
      end
    end

Now it works with:

  • host info --id 4 --fields 'id,name,network'
  • host info --id 4 --fields 'id,name,network*'
  • host info --id 4 --fields 'id,name,network/domain'

What do you think?

Copy link
Member

left a comment

This works pretty well now!
There are two things left for later:

Thanks @ofedoren! 🍺

@mbacovsky mbacovsky merged commit 4b12b87 into theforeman:master Jun 5, 2019
2 checks passed
2 checks passed
Redmine issues Valid issues
Details
hammer Build finished. 2379 tests run, 0 skipped, 0 failed.
Details
@tstrachota

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Congratulations @ofedoren 🍻! It was very close to celebrate a one-year anniversary of this PR! 🎂 🔨 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.