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.

Sign up for GitHub

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

Jump to bottom

Fix snake conversion #2842

Merged
merged 9 commits into from
Apr 17, 2020
Merged

Fix snake conversion #2842

merged 9 commits into from
Apr 17, 2020

Conversation

camilamacedo86
Copy link
Contributor

Description of the change:
Fix snake conversion

Motivation for the change:
Closes #2393

@camilamacedo86 camilamacedo86 changed the title [Ansible based-oprator] - Fix snake conversion Fix snake conversion Apr 16, 2020
@camilamacedo86 camilamacedo86 added language/ansible Issue is related to an Ansible operator project kind/bug Categorizes issue or PR as related to a bug. labels Apr 16, 2020
fabianvf
fabianvf reviewed Apr 16, 2020
View reviewed changes
pkg/ansible/paramconv/paramconv_test.go
},
{
name: "should convert to Snake when has numbers",
args: args{"k8s_var"},
Copy link
Member

Choose a reason for hiding this comment

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

This should be camelCase, eg k8sVar, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is a unit test for the snake only,
We can add this test in the testToCamel added here to check as well. I will do.

Copy link
Member

Choose a reason for hiding this comment

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

But the toSnake tests should be testing from camelCase to snake_case I think, I only see tests that are doing snake_case -> snake_case

fabianvf
fabianvf approved these changes Apr 17, 2020
View reviewed changes
Copy link
Member

@fabianvf fabianvf left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@jmrodri
Copy link
Member

jmrodri commented Apr 17, 2020

The tests in TestToSnake aren't that extensive. We're passing in var, 888, k8s_var, _k8s_var and k8s var. Three of these are already snake case. var, k8s_var and _k8s_var are all no-op test cases since they are already in snake_case.

I think better test would be:

  • ThisShouldHaveUnderscores -> this_should_have_underscores
  • k8sVar -> k8s_var
  • _CanYou_Handle_mixedVars -> _can_you_handle_mixed_vars
  • this_should_be_a_noop -> this_should_be_a_noop

jmrodri
jmrodri requested changes Apr 17, 2020
View reviewed changes
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

@camilamacedo86 I think we need to change the TestToSnake to have better input examples. The rest of these tests look okay.

@camilamacedo86
Copy link
Contributor Author

Hi @jmrodri.

Really tks for the input. Added the missing scenarios.

@jmrodri jmrodri merged commit 37d12b5 into operator-framework:master Apr 17, 2020
@camilamacedo86 camilamacedo86 deleted the fix-2393 branch April 17, 2020 17:14
@estroz
Copy link
Member

estroz commented May 13, 2020

/cherry-pick v0.17.x

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request May 13, 2020
Merged
@openshift-cherrypick-robot
Copy link

@estroz: new pull request created: #3026

In response to this:

/cherry-pick v0.17.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrodri jmrodri jmrodri approved these changes

@fabianvf fabianvf fabianvf approved these changes

@asmacdo asmacdo Awaiting requested review from asmacdo

Assignees

@fabianvf fabianvf

Labels
kind/bug Categorizes issue or PR as related to a bug. language/ansible Issue is related to an Ansible operator project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ansible and inserted underscores in extravars
6 participants
@camilamacedo86 @openshift-ci-robot @jmrodri @estroz @openshift-cherrypick-robot @fabianvf