Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Properly handle resources with '-' in the group name #2712

Merged
merged 4 commits into from
Mar 26, 2020

Conversation

fabianvf
Copy link
Member

Resources with '-' in the APIGroup now are properly formatted when
passed to Ansible. For example

apiVersion: cert-manager.io/v1alpha2
kind: Certificate

will now result in a variabled called
_cert_manager_io_certificaterequest
instead of
_cert-manager_io_certificaterequest

Fixes #2486

Description of the change:

Motivation for the change:

Resources with '-' in the APIGroup now are properly formatted when
passed to Ansible. For example

```
apiVersion: cert-manager.io/v1alpha2
kind: Certificate
```

will now result in a variabled called
`_cert_manager_io_certificaterequest`
instead of
`_cert-manager_io_certificaterequest`

Fixes operator-framework#2486
Copy link
Contributor

@djzager djzager left a comment

Choose a reason for hiding this comment

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

Maybe update runner_test.go with a TestMakeParameters?

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

+1 for @jmrodri suggestion and add changelog :-)
Also, could we add any data mock test for this scenario easily?
Other

Copy link
Member

@estroz estroz 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 lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Mar 25, 2020
@fabianvf fabianvf force-pushed the ansible-escape-dash branch from 9edeeda to 704f941 Compare March 25, 2020 19:39
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

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

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2020
@fabianvf fabianvf merged commit 97594dc into operator-framework:master Mar 26, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resources that use - in the API group name can't be directly accessed by ansible
6 participants