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

add full reference to CIDR_OR_ID_MATCHER #13597

Closed
wants to merge 1 commit into from
Closed

add full reference to CIDR_OR_ID_MATCHER #13597

wants to merge 1 commit into from

Conversation

jntullo
Copy link

@jntullo jntullo commented Jan 20, 2017

Without the full reference to CIDR_OR_ID_MATCHER, tagging API is returning uninitialized constant Api::Subcollections::Tags::CID_OR_ID_MATCHER errors (as was referenced in #13581).

cc: @imtayadeway
@miq-bot add_label bug, api
@miq-bot assign @abellotti

@miq-bot
Copy link
Member

miq-bot commented Jan 20, 2017

Checked commit jntullo@90406ba with ruby 2.2.6, rubocop 0.46.0, and haml-lint 0.19.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

@abellotti
Copy link
Member

Can you add one test in this PR that exercises the fix ? Thanks.

@abellotti abellotti self-assigned this Jan 20, 2017
@imtayadeway
Copy link
Contributor

@jntullo I'm curious if the other places that reference this constant in the API are broken? Also, did you find out where/why this regressed?

@isimluk
Copy link
Member

isimluk commented Jan 20, 2017

CIDR_OR_ID_MATCHER is also present in CompressedIds. The compressedIds seem to be still included in the same final class.

Perhaps it is now included in different order than it used to?

@Fryguy
Copy link
Member

Fryguy commented Jan 20, 2017

@imtayadeway Has the PR for the move away from ApplicationRecord been merged yet? If so, could that be responsible?

@imtayadeway
Copy link
Contributor

@Fryguy it hasn't....probably need to go over that carefully before that gets merged. I suspect this is either a) ordering of includes in Api::BaseController, which may have been changed a while back when doing some housekeeping in that file, or b) some weirdness to do with constant lookup inside mixed-in modules, the structure of which changed when we broke out into separate controllers. I'm curious why the references in app/controllers/api/base_controller/parser.rb and
app/controllers/api/subcollections/policies.rb don't appear to have broken (or don't have coverage?). @jntullo LMK if you want me to look into this....I am putting myself as the leading suspect 😉

@jntullo
Copy link
Author

jntullo commented Jan 20, 2017

@imtayadeway investigating now 😄

@jntullo
Copy link
Author

jntullo commented Jan 20, 2017

@imtayadeway I believe it has to do with the reordering, and I am worried that some things don't have coverage (going to work on that). When include CompressedIds is added to Api::Subcollections::Tags, it also solves the problem

@jntullo jntullo mentioned this pull request Jan 20, 2017
Copy link
Contributor

@imtayadeway imtayadeway left a comment

Choose a reason for hiding this comment

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

@jntullo could you add a failing test for this, and also incorporate the changes to the order of includes as discussed above? Thanks!

EDIT: I believe there are several places where we are unnecessarily including that module into different parts of the API controller because of reasons above. I believe they could also be removed as part of this PR.

@jntullo
Copy link
Author

jntullo commented Jan 24, 2017

@imtayadeway can do, but was also waiting for feedback on #13581 where I have refactored this line out.

@imtayadeway
Copy link
Contributor

@jntullo I spent some time digging into this today and made a PR based on my findings in #13671. LMK what you think!

@abellotti
Copy link
Member

Closing in preference of merged #13671

@abellotti abellotti closed this Jan 27, 2017
@miq-bot
Copy link
Member

miq-bot commented Jan 27, 2017

This pull request is not mergeable. Please rebase and repush.

@jntullo jntullo deleted the bug/cidr_or_id_matcher branch November 28, 2017 19:43
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.

6 participants