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 #22696 - Fix errors when --packages is used #563

Merged
merged 1 commit into from
Jul 16, 2018

Conversation

shiramax
Copy link
Contributor

No description provided.

@theforeman-bot
Copy link

Issues: #22696

@akofink akofink self-requested a review June 19, 2018 17:26
@akofink
Copy link
Member

akofink commented Jun 19, 2018

Thanks for contributing! :)

This issue stems from the fact that the packages API uses search instead of the name parameter directly (i.e. see the products controller as an example). This is the way that all of core Foreman's index APIs are, but Katello has a mixed API, unfortunately (believe me, there is always ranting happening about this, but it's a lot of work to unify the search pattern).

Anyway, the easiest way to handle this is to use the lib/hammer_cli_katello/foreman_search_options_creators.rb. This will tell hammer-cli-katello to treat packages like a Foreman API resource for searching.

key_names = HammerCLI.option_accessor_name 'names'
key_organization_id = HammerCLI.option_accessor_name 'organization_id'
options[key_organization_id] ||= organization_id(scoped_options('organization', options))
find_resources(:packages, options).select { |package| options[key_names].include? package['name'] }.map { |package| package['id'] }
Copy link
Member

Choose a reason for hiding this comment

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

This line is too long, according to rubocop (that's why the build is failing).

@shiramax
Copy link
Contributor Author

@akofink thanks :) please review again .

Copy link
Member

@akofink akofink left a comment

Choose a reason for hiding this comment

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

Works as expected. Would you mind adding a test for this bug to test/functional/content_view/version/incremental_update_test.rb?

@shiramax
Copy link
Contributor Author

shiramax commented Jul 9, 2018

@akofink Done, please review again :)

@@ -88,4 +88,18 @@
result = run_cmd(@cmd + params)
assert_equal(result.exit_code, 0)
end

it "performs incremental update with packages" do
params = ['--content-view-version-id=5', '--package-ids=1', '--lifecycle-environment-ids=1']
Copy link
Member

Choose a reason for hiding this comment

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

We want to test the --packages option specifically rather than --package-ids. This will create another request to packages#index.

@shiramax
Copy link
Contributor Author

@akofink, thanks.
please review again :)

Copy link
Member

@akofink akofink left a comment

Choose a reason for hiding this comment

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

Just a couple other comments about tests.

).at_least_once

ex = api_expects(:content_view_versions, :incremental_update, 'Incremental Update') do |par|
par[:content_view_version_environments][0][:content_view_version_id] == 5
Copy link
Member

Choose a reason for hiding this comment

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

The package ids returned from the previous search should be passed in here.


expect_generic_search(
:packages, params: {search: "name = \"bla\""}, returns: {'id' => 5}
).at_least_once
Copy link
Member

Choose a reason for hiding this comment

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

at_least_once allows for multiple searches. It should work without it (thus expecting only one call).

@shiramax shiramax force-pushed the 22696 branch 3 times, most recently from 147c41c to e67c31d Compare July 12, 2018 12:34
@shiramax
Copy link
Contributor Author

@akofink thanks!
please review again :)

Copy link
Member

@akofink akofink left a comment

Choose a reason for hiding this comment

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

@shiramax 🙈 a few more comments here

par[:content_view_version_environments][0][:content_view_version_id] == 5 &&
par["add_content"]["package_ids"] == [5]
end
ex.returns(index_response([{'id' => 5}]), 'id' => '3', 'state' => 'stopped')
Copy link
Member

Choose a reason for hiding this comment

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

The index_reponse should be on line 96, not here. You only need the 'id' => '3' here.

params = ['--content-view-version-id=5', '--packages=bla', '--lifecycle-environment-ids=1']

expect_generic_search(
:packages, params: {search: "name = \"bla\""}, returns: {'id' => 5}
Copy link
Member

Choose a reason for hiding this comment

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

This test would be more clear if the package ID didn't match the content_view_version_id. Just pick a different number.

@shiramax
Copy link
Contributor Author

@akofink thanks ! done

Copy link
Member

@akofink akofink left a comment

Choose a reason for hiding this comment

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

Almost there, just one more thing, and I think it'll be good to go.


expect_generic_search(
:packages, params: {search: "name = \"bla\""}, returns: {'id' => 15}
).returns(index_response([{'id' => 15}]))
Copy link
Member

Choose a reason for hiding this comment

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

You can put the returns here or on line 96, but you don't need it in both places. I think the last one gets honored, so the returns: {'id' => 15} isn't doing anything.

@shiramax
Copy link
Contributor Author

@akofink done :)

Copy link
Member

@akofink akofink left a comment

Choose a reason for hiding this comment

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

Looks good! ACK! Thanks @shiramax!

@akofink akofink merged commit f757628 into Katello:master Jul 16, 2018
jturel pushed a commit to jturel/hammer-cli-katello that referenced this pull request Sep 6, 2018
akofink pushed a commit that referenced this pull request Sep 7, 2018
* Fixes #22696 - Fix errors when --packages is used (#563)

(cherry picked from commit f757628)

* fixes #23993 - fixes entity filtering by adding searchables (#574)

(cherry picked from commit 31e2fb8)

* Fixes #24523: s/smart_proxy/Content Source/ (#573)

When users provide `--content-source foo`, they should receive back
error messages talking about content source, not smart proxies.

(cherry picked from commit 29a8570)

* Bump version to 0.13.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants