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

Simpler derivation of singular & plural method names #366

Merged
merged 12 commits into from
Nov 26, 2018

Conversation

cben
Copy link
Collaborator

@cben cben commented Nov 22, 2018

Simpler logic, more explicit about intent (e.g. for the 2 special kinds hardcode the final result instead of intermediate step), added explanations.

Also fixes #307 - get_security_context_constraints.kind, get_endpoints.kind are now plural as in kubernetes.
Also fixes #367 - create_security_context_constraint now works. (tested live too)

Added lots of tests.

  • added changelog.

@f4tq @eatwithforks please review (easier by commits).

After this, I'll rebase #355 and change it to support both new and old names...

@cben cben force-pushed the refactor-singular-plural branch from 18e10d5 to e728976 Compare November 23, 2018 08:57
@cben
Copy link
Collaborator Author

cben commented Nov 23, 2018

There was also a workaround for create_endpoint that I now removed, but was missing for create_security_context_constraint, so I believe this PR also fixed that, adding test...
UPDATE: tested live, it really didn't work => #367, now it really does.

@cben
Copy link
Collaborator Author

cben commented Nov 23, 2018

All done now, PTAL.
(or enjoy holidays :-)

cben added 11 commits November 26, 2018 13:45
security.openshift.io_api_resource_list.json taken from openshift v3.10.0.
(openshift also includes SecurityContextConstraints also in api/v1/,
but I prefer to keep core_api_resource_list.json pure upstream k8s
and separate openshift-specific test stuff.)

kubernetes/kubernetes#8115
Similar to Endpoints we force this kind to singular method names.
Unlike Endpoint, lacked a workaround to pass original kind when creating.
ManageIQ#367

Also fixes ManageIQ#307 - get_security_context_constraints.kind,
  get_endpoints.kind are now plural as in kubernetes.
Also fixes ManageIQ#367 - create_security_context_constraint now works.
Workaround was also missing for SecurityContextConstraints,
which probably means create_security_context_constraint was broken.
@cben cben force-pushed the refactor-singular-plural branch from 1e2319b to 8c481f2 Compare November 26, 2018 11:48
@cben
Copy link
Collaborator Author

cben commented Nov 26, 2018

@f4tq @eatwithforks @grosser @NickLaMuro @agrare looking for someone to review this...

@cben cben merged commit 5c4b298 into ManageIQ:master Nov 26, 2018
@cben cben mentioned this pull request Nov 28, 2018
@cben
Copy link
Collaborator Author

cben commented Dec 10, 2018

Ouch, I think I caused a regression here for plurals which are not a pure suffix.
For example kind: WordBoundary, plural name: wordboundaries.
The previous algorithm only assumed a match between WordB and wordb and would have worked. The new one expects "wordboundaries".start_with?("wordboundary") which doesn't hold :-(
Well now I know why the strange till-last-capital-letter was there 🤦‍♂️

cc @masayag this might be a blocker for #373 — I'd prefer not to complicate it further before we fix this.

@cben
Copy link
Collaborator Author

cben commented Dec 12, 2018

opened issue #376 with exact details. will fix ASAP, but having a very busy week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants