[anandjambhulkar]

Summary

• hammer location delete --id 4,5 now deletes each listed location
• Single --id and --name behavior is unchanged

Related issue

Fixes https://projects.theforeman.org/issues/38182

Test plan

• Tested on Satellite: hammer location delete --id <id1>,<id2> deletes both
• Single --id still works
• Delete by --name still works
• bundle exec rake test TEST=test/functional/location_test.rb

[adamruzicka]

Are we absolutely sure this is a good idea? This would introduce an inconsistency where some options accept --id 1 (singular), some accept --id 1,2 (singular, but a list) and others accept explicitly --ids 1,2 (plural, with a list)

[ofedoren]

Even though the issue suggests for this option to support multiple ids, it's not something that we do. For multiple ids we have --<resource>-ids options where applicable.

This command maps the API endpoint: 1 location deletion per 1 call. Same applies to this command. In other words: this is not the solution.

When I re-read the issue, it says that this option is a bit confusing since it doesn't check for the actual value was provided, and implicitly takes the first number it can.

For that reason, we have option normalizers, such as https://github.com/theforeman/hammer-cli/blob/master/lib/hammer_cli/options/normalizers.rb#L200

Ideally this option should already have some automatically assigned normalizer/validator per API description, so the same id value can be used for API call directly or as option in hammer. Currently, the endpoint documents this as identifier: https://github.com/theforeman/foreman/blob/develop/app/controllers/concerns/api/v2/taxonomies_controller.rb#L90, which means this: https://github.com/theforeman/foreman/blob/develop/config/initializers/apipie.rb#L123

If we really want to fix this properly, we shouldn't just add support for multiple ids for this option, we need to ensure this option behaves the same way as the corresponding API param. Worst case, I'd image this option being "hardcoded" as numerical, but it's also worth revisiting the API side and actually check/ask: do we really need/support identifier type as location ids.

[anandjambhulkar]

Hello @adamruzicka @ofedoren.

Thanks for the clarification — I misunderstood the issue as “support multiple deletes,”. However, the real problem is that --id , is accepted and only the first id is used.
I’ll drop the multi-delete approach and instead validate --id with a Number normalizer so invalid values like , fail for sure. Can you please confirm that this matches what you’d like for this PR?

[ofedoren]

Thanks, @anandjambhulkar !

Can you please confirm that this matches what you’d like for this PR?

More or less -- yes. My only concern that API documents this parameter/option as identifier, which also supports 1-name string as a valid value, so if the API is actually correctly accepts such parameter, hammer should reflect that as well instead of simply denying such value.

So in other words: API is the source of truth hammer must follow. If API accepts strings as IDs, hammer should also. You can take a look how we deal with other such parameters/options.