Add install mise ruby_manager and ruby (when we get around to this) - #455
Add install mise ruby_manager and ruby (when we get around to this)#455ianheggie-oaf wants to merge 3 commits into
Conversation
|
@benrfairless or @Br3nda - I added this for completeness on my own time, but a discussion needs to be held which ruby manager we want to standardise on particularly for production, but also first class support in development? mise, mise with nix plugin, nix? (Personally I prefer the dead simple .ruby-version and .python-version which are tool agnostic (I use mise), direnv for ENV vars, and then @Br3nda 's Makefile approach for bootstrapping common tasks) |
a29daf7 to
97e16f4
Compare
|
@benrfairless - in preparation of moving to mise for server as well as dev systems in the future |
| - name: Install mise | ||
| apt: | ||
| name: mise | ||
| state: present | ||
| update_cache: yes |
There was a problem hiding this comment.
Bug: The remove_mise role does not uninstall the mise apt package, leaving orphaned packages on systems that switch away from using mise.
Severity: LOW
Suggested Fix
Add a task to the internal/remove_mise role to uninstall the mise apt package using apt: name: mise state: absent. Also, consider adding a task to remove the mise apt repository to ensure a complete cleanup.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: roles/internal/mise/tasks/main.yml#L27-L31
Potential issue: When a host is configured to use `mise` and then later switched to a
different `ruby_manager`, the `internal/remove_mise` role is executed. However, this
role only removes user-specific configuration files and does not remove the system-wide
`mise` apt package or its associated apt repository. This results in orphaned packages
and repositories remaining on the system, which could lead to unexpected behavior or
conflicts in the future.
Did we get this right? 👍 / 👎 to inform future reviews.
| loop: "{{ ruby_versions }}" | ||
|
|
||
| - name: Install bundler for each ruby version | ||
| become: yes | ||
| become_user: deploy | ||
| command: mise exec ruby@{{ item }} -- gem install bundler --no-document | ||
| args: | ||
| creates: "/home/deploy/.local/share/mise/installs/ruby/{{ item }}/bin/bundle" | ||
| loop: "{{ ruby_versions }}" |
There was a problem hiding this comment.
Bug: The role will fail if ruby_manager is set to mise but the ruby_versions variable is not defined for a host, as it lacks a default value.
Severity: HIGH
Suggested Fix
Provide a default empty list for the loop variable to prevent the run from failing when no versions are defined. Change the loop from with_items: "{{ ruby_versions }}" to with_items: "{{ ruby_versions | default([]) }}".
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: roles/internal/mise/tasks/main.yml#L39-L47
Potential issue: The `internal/mise` role iterates over the `ruby_versions` variable to
install specified Ruby versions. However, this variable is not defined globally and
lacks a default value within the role. If a host is configured with `ruby_manager ==
'mise'` but does not have `ruby_versions` defined in its group variables, the Ansible
run will fail with an "undefined variable" error when it attempts to loop over the
non-existent variable.
Did we get this right? 👍 / 👎 to inform future reviews.
benrfairless
left a comment
There was a problem hiding this comment.
You already have an Ansible role called "remove_mise", and I think it would make more sense to have a single role related to the installation, modification and removal of mise.
The comments by Sentry seem to be something that actually needs to be looked at.
The role you've created only has a tasks/main.yml and a minor update to site.yml. It doesn't contain any documentation about what the role is and what it is used for.
Pull request was converted to draft
| - name: Install mise | ||
| apt: | ||
| name: mise | ||
| state: present | ||
| update_cache: yes |
There was a problem hiding this comment.
Bug: The bundle_path for mise is hardcoded to an old path, but the new apt installation places the binary at /usr/bin/mise, which will cause command failures.
Severity: HIGH
Suggested Fix
Update the mise value for ruby_manager_bundle_path in group_vars/all.yml to use the correct path for the apt-installed binary: /usr/bin/mise exec -- bundle.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: roles/internal/mise/tasks/main.yml#L27-L31
Potential issue: The pull request changes the installation method for `mise` to use
`apt`, which places the binary at `/usr/bin/mise`. However, the `bundle_path` variable
in `group_vars/all.yml` is hardcoded to `/home/deploy/.local/bin/mise exec -- bundle`.
Since the old path will no longer exist, any service configured to use `mise` will fail
when attempting to execute bundle or foreman commands, as the `bundle_path` will be
invalid.
Relevant issue(s)
What does this do?
Add install mise ruby_manager and ruby (when a project is changed to use mise),
Why was this needed?
Prepare for future move to mise
Implementation/Deploy Steps (Optional)
ansible provision as normal
Notes to reviewer (Optional)
Should not change anything till a service is changed to use mise