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

Use extra_vars and CostManagement CR to drive data collection. #56

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chambridge
Copy link
Contributor

@chambridge chambridge commented Sep 1, 2020

The ansible operator is passed extra_vars based on the Custom Resource it is reconciling. We should be using the CR data passed to us instead of looking up a defined name.

Use k8s_status to set and capture upload_attempt_time for executing data collection.
Remove usage of CostManagementData for reconciliation.

cost-mgmt-operator-7bb6b87fdd-zz67f.log

@chambridge chambridge force-pushed the current-namespace-cleanup branch from 81aeeff to 4e18d35 Compare September 3, 2020 21:36
@chambridge chambridge changed the title Use ansible_operator_meta to drive interactions. Use extra_vars and CostManagement CR to drive data collection. Sep 3, 2020
@chambridge chambridge force-pushed the current-namespace-cleanup branch from 4e18d35 to a7a060b Compare September 3, 2020 21:48

- name: Format current_month string if less than 10
set_fact:
current_month: '{{ "0" + (current_month | string) }}'
Copy link
Contributor

@abaiken abaiken Sep 8, 2020

Choose a reason for hiding this comment

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

When I run this it results in the following error:

Error was a <class 'ansible.errors.AnsibleError'>, original message: An unhandled exception occurred while templating '{{ current_month }}'. 

ansible/ansible#39369 might have some helpful info

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@abaiken abaiken left a comment

Choose a reason for hiding this comment

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

Left some initial feedback. Overall this approach is looking good - we will just want to test it really well before we merge. There might be some weirdness in getting defaults (I left a comment about the error I'm seeing) so we will need to work through that & there is still some cleanup to do in the tests/README.

roles/collect/tasks/main.yml Outdated Show resolved Hide resolved
roles/collect/tasks/main.yml Outdated Show resolved Hide resolved
@chambridge chambridge marked this pull request as ready for review September 10, 2020 16:57
Copy link
Contributor

@abaiken abaiken left a comment

Choose a reason for hiding this comment

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

This is looking great! Can you remove the CostManagementData CR & CRD from the deploy/crds directory and update the README here https://github.com/project-koku/korekuta-operator#creating-the-dependencies & fix the make command for deploying dependencies.

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

Successfully merging this pull request may close these issues.

2 participants