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

Roles and groups now match service names, only 1 playbook, moved sample inventories into new directory #124

Merged
merged 85 commits into from Aug 22, 2019

Conversation

@domenicbove
Copy link
Contributor

commented Jul 29, 2019

Description

There was duplicating code with use of many all.yml playbooks, slimmed down to 1 playbook file
Moved sample inventories to new directory
Renamed roles, groups, and variables to match the names of the services that get installed
Inventories will need to be updated to match new variables and group names

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Ran new all.yml to install plaintext, kerberos ssl, and ssl confluent platforms

Test Configuration:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules
@domenicbove
Copy link
Contributor Author

left a comment

some big changes in the names of things, inventory groups need updating. broker_id var also needs to change in the inventories. this also fixed a bug with the handlers running in the wrong order. shared variables across the roles are now in group_vars/all/all.yml

@domenicbove domenicbove requested a review from JumaX Jul 30, 2019

@domenicbove domenicbove changed the title refactor to have only 1 playbook and moved sample inventories into new directory Roles and groups now match service names, only 1 playbook, moved sample inventories into new directory Jul 30, 2019

@JumaX
Copy link
Member

left a comment

The change from broker to kafka doesn't make sense, as kafka is not a component but the name of the whole platform. If you want to change it to kafka_broker, like what you've done with kafka_connect that would make sense.

all.yml Outdated Show resolved Hide resolved
all.yml Outdated Show resolved Hide resolved
inventories/kerberos_example.yml Outdated Show resolved Hide resolved
roles/confluent.kafka/handlers/main.yml Outdated Show resolved Hide resolved
roles/confluent.kafka/tasks/main.yml Outdated Show resolved Hide resolved
@JumaX

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Additional notes:

  1. in all.yml, why do we not gather facts?
  2. the all.yml in cp-ansible/group_vars/all contains no suffix and I think that it's called all is a bit confusing relative to the all.yml in the root directory.
  3. Are we doing a separate PR for exposing the variables?
JumaX added 10 commits Jul 31, 2019
deleted: roles/confluent.preflight/handlers/main.yml
	deleted:    roles/confluent.preflight/tasks/debian.yml
	deleted:    roles/confluent.preflight/tasks/main.yml
	deleted:    roles/confluent.preflight/tasks/redhat.yml
	deleted:    roles/confluent.preflight/tasks/ubuntu.yml

Removed preflight role.
modified: tasks/debian.yml
	modified:   tasks/redhat.yml
Removed package installation.
modified: debian.yml
	modified:   redhat.yml

Added installation of confluent-kafka and confluent-server packages based on security settings.
modified: debian.yml
	modified:   redhat.yml

Configured standard package installation.
modified: debian.yml
	modified:   redhat.yml

Corrected package name.
@domenicbove
Copy link
Contributor Author

left a comment

im happy

@JumaX
JumaX approved these changes Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.