-
Notifications
You must be signed in to change notification settings - Fork 4
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
Remove ansible vault requirement #292
Conversation
The docke rlogin isnt needed as the images are publicly available. Removing the inclusion of vault/keepass references
It was ssumed the default user will always be called "ubuntu" Allow the default user to be updated and added to the list of docker users The key is excluded as the default user is already added to the authorised leys list, they just need to be assigned to the docker group
If the user doesnt have an SSH key defined, skip the step for adding the key to authorised users list
This reverts commit 314a797.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, has this been tested a proved working with a platform deploy done? I didn't run this myself.
Hi @rcrichton Ive tested this successfully within my local environment making use of a local VM to run the ansible script against. Below is a screencast of this being tested (making use of a multipass VM instance) Screencast.from.08-05-2024.10.05.23.webm |
@coderabbitai review |
Actions PerformedReview triggered.
|
WalkthroughThe changes streamline Ansible configurations for Docker setups by removing unnecessary tasks and credentials, updating user entries, and refining conditions within playbooks. Vault-related sections have been eliminated for better security, and instructions updated accordingly. Major adjustments include adding and configuring the Changes
Poem
Tip AI model upgrade
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Out of diff range and nitpick comments (5)
infrastructure/ansible/README.md (5)
Line range hint
6-6
: Consider revising the repeated phrase "Ansible Docker Community Collection installed" to enhance clarity.- Ansible Docker Community Collection installed + Ansible Docker Community Collection is installed
Line range hint
23-23
: Add a comma after "all.yml_" for better readability.- all.yml_. To have docker access you need to add your ssh key to the `docker_users` var in the _/inventories/{ENVIRONMENT}/group_vars/all.yml_. + all.yml_, To have docker access you need to add your ssh key to the `docker_users` var in the _/inventories/{ENVIRONMENT}/group_vars/all.yml_.
Line range hint
25-25
: Consider rephrasing to avoid repetitive wording and enhance readability.- An authorised user will need to run the `provision_servers.yml` playbook to add your ssh key to the servers. + An authorised user must execute the `provision_servers.yml` playbook to add your ssh key to the servers.
Line range hint
29-29
: Correct the verb form for better grammatical accuracy.- else ansible might complains, for each server run: + else ansible might complain, for each server run:
Line range hint
35-35
: Insert a comma after "known_hosts" for improved readability.- ~/.ssh/known_hosts ``` To run a playbook you should do: + ~/.ssh/known_hosts, ``` To run a playbook you should do:
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- infrastructure/ansible/README.md (2 hunks)
- infrastructure/ansible/inventories/development/group_vars/all.yml (2 hunks)
- infrastructure/ansible/roles/common/tasks/main.yml (1 hunks)
- infrastructure/ansible/roles/docker/tasks/main.yml (2 hunks)
Files skipped from review due to trivial changes (2)
- infrastructure/ansible/inventories/development/group_vars/all.yml
- infrastructure/ansible/roles/docker/tasks/main.yml
Additional Context Used
LanguageTool (5)
infrastructure/ansible/README.md (5)
Near line 6: Possible typo: you repeated a word
Context: ...s - Linux OS to run commands - Install Ansible - Ansible Docker Community Collection installed ...
Near line 23: Possible missing comma found.
Context: ...T}/group_vars/all.yml_. To have docker access you need to add your ssh key to the `d...
Near line 25: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...vars/all.yml_. An authorised user will need to run theprovision_servers.yml
playboo...
Near line 29: The modal verb ‘might’ requires the verb’s base form.
Context: ...our known hosts file else ansible might complains, for each server run: ```sh ssh-keysca...
Near line 35: It seems that a comma is missing.
Context: ...st> >> ~/.ssh/known_hostsTo run a playbook you should do:
bash ansible-playboo...
Additional comments not posted (1)
infrastructure/ansible/roles/common/tasks/main.yml (1)
34-34
: Ensure that the conditionitem.key is defined
is correctly placed to prevent errors during execution when certain expected variables are undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- infrastructure/ansible/inventories/development/group_vars/all.yml (2 hunks)
Additional comments not posted (1)
infrastructure/ansible/inventories/development/group_vars/all.yml (1)
37-38
: Verify the correctness of the new user entry.The new entry with the username
ubuntu
has been added to thedocker_users
list. Ensure that this user exists and is intended to be part of thedocker_users
list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The Vault is no longer needed as the docker login is also not needed to execute the ansible script.
This is currently also a blocker for anyone wanting to use ansible to provision the servers and they dont have access to keepass to retrieve the vault password
Additional update was to make the default user configurable within the inventory. It was assumed the default user will always be
ubuntu
, and statically adding theubuntu
user to the docker group. This is now part of the docker_users list to dynamically define/update the default user in useSummary by CodeRabbit
Documentation
--ask-vault-pass
option and eliminating Vault and Keepass sections.New Features
ubuntu
to thedocker_users
list.Refactor
ubuntu
user management and Docker Hub login.Chores