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 #19574 - Validator uses options from option collector #242

Merged
merged 2 commits into from Dec 17, 2018

Conversation

Projects
None yet
3 participants
@tstrachota
Copy link
Member

tstrachota commented May 17, 2017

In 5bf5073 Hammer introduced option sources that enable reading option values from additional providers. Default options have been turned into an option source too. Validator currently reads defaults directly. It should use option sources instead, otherwise it will prevent a command to start when required options come from some additional option source (potentially added in future).

@tstrachota

This comment has been minimized.

Copy link
Member Author

tstrachota commented May 17, 2017

Hold on with merging this. I just found out it breaks two tests in hammer-cli-katello. Override of all_options in ContentViewNameResolvable raises a "Missing options to search organization" before the validator exception is raised:
https://github.com/Katello/hammer-cli-katello/blob/master/lib/hammer_cli_katello/content_view_name_resolvable.rb

@tstrachota tstrachota force-pushed the tstrachota:validator_options branch from 1470e95 to 2028e56 Dec 4, 2018

@tstrachota

This comment has been minimized.

Copy link
Member Author

tstrachota commented Dec 4, 2018

The original commit that fixes #19574 wasn't compatible with most of option validations and tests in Katello that expect ID resolution to happen after the validations. To get it working I added changes allowing to place validation block between option sources #22253. It's several commits and we'll have to squash that prior to merge.

Changes in the core I made:

  • moved the logic for merging options from OptionCollector to a new class SourcesList, it has the same interface for getting the options as an ordinary option source allowing to nest them.
  • SourcesList can execute validations
  • extracted the logic for inserting new items after and before a certain item in an array, moved it from output and help definition into a helper insert_relative
  • each option source and validation can have a name allowing to look it up later and place a new source/validation before/after.
  • enhanced validate_options to allow relative inserts, usage example:
validate_options(:after, 'UserParams') do
  # ...inserts the validation block after UserParams option source
end
# or
validate_options(:after, 'UserParams', validator: Custom::Validator.new)

validate_options(:append) do
  # ...adds validation to the end of the queue (default behavior)
end
# or
validate_options(:append, validator: Custom::Validator.new)

# Similar to inserting output or help blocks, following modes can be used:
# before, after, append, prepend, replace

The intended structure of option sources in hammer-cli-foreman is:

- DefaultInputs
    - CommandLine
    - SavedDefaults
- IdResolution
    - IdParams
    - IdsParams
    - SelfParam

The default behavior always adds the block at the very end of the queue so that it validates already resolved options. If a different behavior is required, a validation can be placed before the id resolution with:

validate_options :before, 'IdResolution' do
end

Changes in plugins

These are changes that have to be made in plugins after this PR is merged:

Other plugins might need changes too.

Todo

  • tests for new parameters of validate_options
  • tests for insert_relative
  • tests for ProcessorList
  • update docs

Please review the design and test the behavior. I'll work on the tests in the meantime.

@tstrachota

This comment has been minimized.

Copy link
Member Author

tstrachota commented Dec 4, 2018

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Dec 7, 2018

@tstrachota, thanks for this PR! It works pretty well! I like the nested structure of option sources, it is quite flexible and easy to extend. As per our discussion I'd like to suggest a few changes:

  • option sources and validators should have common predecessor e.g. OptionProcessor. It should have @name and process(option_definitions, option_values). The process should call or better replace the get_options and run in its respective successors.
  • SourcesList could be renamed to ProcessorList or ProcessorGroup
  • OptionCollector needs to be updated accordingly
  • there is HammerCLI::Options::Sources and HammerCLIForeman::OptionSources Do we want to have them consistent?
  • I'm not sure what to do with the instance method validate_options. We probably want to keep it and document the cases when it is preferred over the class method. The difference is:
    • it is inheritable
    • it is always evaluated after all the processors and can not change the order
    • it is evaluated in a scope of command and can condition the validations according to the command state. (the class method block is evaluated in a scope of the DSL class and have no access to commands attributes other then the options)

Also some docs needs to be updated:

@tstrachota tstrachota force-pushed the tstrachota:validator_options branch from 255708b to 3a5b740 Dec 13, 2018

@tstrachota

This comment has been minimized.

Copy link
Member Author

tstrachota commented Dec 13, 2018

@mbacovsky updated according to your comments. I also added tests and some docs.
It's now ready for another round of review.

#### Default option sources

Abstract Hammer command uses two default option sources -
`HammerCLI::Options::Sources::CommandLine` responsible for intial population of the options and
`HammerCLI::Options::Sources::SavedDefaults` adding defaults managed by the `defaults` command.

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Dec 14, 2018

Member

Could we add something like: The default option sources are wrapped in DefaultInputs source list so that the default hierarchy of sources is

DefaultInputs
  CommandLine
  SavedDefaults
assert_equal 'DefaultInputs', target
assert validator, custom_validator
end
end
end

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Dec 14, 2018

Member

Would it make sense to add a test for add_validator method as it is documented extension point?

@tstrachota

This comment has been minimized.

Copy link
Member Author

tstrachota commented Dec 14, 2018

@mbacovsky ok, I can add that.

I've opened PRs with changes required for the Foreman and Katello:
theforeman/hammer-cli-foreman#402
Katello/hammer-cli-katello#608

@tstrachota

This comment has been minimized.

Copy link
Member Author

tstrachota commented Dec 14, 2018

@mbacovsky updated

it 'allows for creating inheritable validators' do
cmd = Class.new(HammerCLI::AbstractCommand)
cmd.send(:define_method, :add_validators) do |sources|
[ stub('Validator1', :name => 'Validator1') ]

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Dec 16, 2018

Member

@tstrachota this stub needs to be executed outside of the block otherwise the test fails.

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Dec 16, 2018

Member
      validator1 = stub('Validator1', :name => 'Validator1')
      cmd = Class.new(HammerCLI::AbstractCommand)
      cmd.send(:define_method, :add_validators) { |sources| [ validator1 ] }
@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Dec 16, 2018

@tstrachota, it looks almost perfect now. Could you fix the failing test?

@tstrachota tstrachota force-pushed the tstrachota:validator_options branch from 69f8395 to f8616e0 Dec 17, 2018

@tstrachota

This comment has been minimized.

Copy link
Member Author

tstrachota commented Dec 17, 2018

@mbacovsky sorry for the test I missed. I fixed that and squashed all the work into 2 commits.

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Dec 17, 2018

Yes
👍 Awesome!
I'll retest with the other PR's. Thanks, @tstrachota!

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Dec 17, 2018

Well done, good to go. Thanks @tstrachota!

@mbacovsky mbacovsky merged commit 23707d1 into theforeman:master Dec 17, 2018

1 check passed

hammer Build finished. 2259 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.