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 6343 - params from searchables are not wrapped #134

Merged
merged 2 commits into from
Jul 23, 2014

Conversation

mbacovsky
Copy link
Member

Options from searchables are now added to params using method_options that should wrap them correctly according to the api docs.

@mbacovsky
Copy link
Member Author

Requires theforeman/hammer-cli#122 to work

@tstrachota
Copy link
Member

@komidore64 would you mind looking at this one? I'm the original author of the idea @mbacovsky implemented and I don't feel right reviewing it. The same for the referenced hammer-cli part, please.
Thank you!

@domcleal
Copy link
Contributor

domcleal commented Jul 7, 2014

@mbacovsky note, missing '#' from commit message for redmine to find it

@mbacovsky
Copy link
Member Author

@domcleal ty, message is fixed

@komidore64
Copy link
Contributor

i'm going to smoke test this and theforeman/hammer-cli#122 with some katello specific commands before acking. thanks.

@tstrachota
Copy link
Member

@mbacovsky I ran hammer-tests on your branches. There's several breakages:

hammer organization add-user --name org16 --user some_user16
Could not associate the user:
    missing param 'id' in parameters
hammer os add-architecture --id 3 --architecture poiuytrdsadsadsad
Architecture has been associated

...but actually it wasn't associated. It sends wrong parameters. I suspect other associating commands are affected as well.

hammer --csv template create --name tpl16 --file ~/template.txt --type provision
Could not create the config template:
    Template kind can't be blank

@komidore64
Copy link
Contributor

seems to work fine for me. i didn't try any of the associated commands that @tstrachota is referring to, tho

@mbacovsky
Copy link
Member Author

I upadated both PRs. The issues Tomas found were fixed.

@@ -56,7 +56,7 @@ class CreateCommand < HammerCLIForeman::CreateCommand
failure_message _("Could not create the operating system")

def request_params
params = method_options
params = method_options(options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are these not calling super ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I'll fix it

@komidore64
Copy link
Contributor

ack pending green tests

@mbacovsky
Copy link
Member Author

[test]

mbacovsky added a commit that referenced this pull request Jul 23, 2014
Fixes 6343 - params from searchables are not wrapped
@mbacovsky mbacovsky merged commit 4a4ef93 into theforeman:master Jul 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants