Skip to content

Use k8s_info module instead of deprecated k8s_facts in molecule scaffold (#2168) #2203

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

Merged
merged 3 commits into from
Nov 26, 2019
Merged

Conversation

geerlingguy
Copy link
Contributor

Description of the change:
Replaces uses of the deprecated k8s_facts module with k8s_info, which is the standard module for k8s information lookups in Ansible 2.9+.

Motivation for the change:

Closes #2168

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 15, 2019
@fabianvf
Copy link
Member

Do you mind making sure that the various Dockerfiles (in ci/dockerfiles as well as internal/scaffold/ansible/dockerfilehybrid.go) are explicitly set to pull in >= 2.9?

@geerlingguy
Copy link
Contributor Author

@fabianvf - ah, completely forgot about that. Bumped minimum to 2.9 instead of 2.8, and I think I covered the only three places that's required.

@fabianvf
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2019
@openshift-ci-robot
Copy link

@geerlingguy: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws-ansible d2aeefc link /test e2e-aws-ansible

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

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 /approved

Thank you for your contribution 🥇

CHANGELOG.md Outdated
@@ -5,6 +5,7 @@

### Changed
- Upgrade minimal Ansible version in the init projects from `2.4` to `2.6`. ([#2107](https://github.com/operator-framework/operator-sdk/pull/2107))
- Use `k8s_info` module instead of deprecated `k8s_facts` module in molecule test scaffold. ([#2168](https://github.com/operator-framework/operator-sdk/issues/2168))
Copy link
Contributor

@camilamacedo86 camilamacedo86 Nov 16, 2019

Choose a reason for hiding this comment

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

Just a nit after the is OK to be merged 👍

Suggested change
- Use `k8s_info` module instead of deprecated `k8s_facts` module in molecule test scaffold. ([#2168](https://github.com/operator-framework/operator-sdk/issues/2168))
- Replace in the ansible based operators module tests the `k8s_facts` for `k8s_info` which is deprecated. ([#2168](https://github.com/operator-framework/operator-sdk/issues/2168))
- Upgrade the ansible version from `2.8` to `2.9` on the ansible based operators image. ([#2168](https://github.com/operator-framework/operator-sdk/issues/2168))

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2019
@geerlingguy
Copy link
Contributor Author

@camilamacedo86 - Made those changes, thanks for pointing out the missing Changelog entry too!

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 Nov 21, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2019
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@geerlingguy
Copy link
Contributor Author

Rebased due to CHANGELOG conflicts, as well as some conflicts due to adding jmespath to the Dockerfiles in #2027

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.

Need fix issue caused by rebasing with master.

@geerlingguy
Copy link
Contributor Author

@camilamacedo86 - Yikes, don't know how that made it through! I have fixed it, sorry about that.

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 /approved

@camilamacedo86 camilamacedo86 merged commit cf1ca34 into operator-framework:master Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch Ansible test scaffold from k8s_facts to k8s_info module
4 participants