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

Add rbenv to Jenkins nodes #2022

Merged
merged 2 commits into from
Mar 19, 2024
Merged

Add rbenv to Jenkins nodes #2022

merged 2 commits into from
Mar 19, 2024

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Jan 30, 2024

No description provided.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Can this use rbenv and ruby-build-rbenv from EPEL? I generally feel more comfortable with packages than git based installations.

I'm tempted to write our own module that only uses packages. #2023 is probably that.

puppet/modules/slave/manifests/unittests.pp Outdated Show resolved Hide resolved

rbenv::build { '3.1.0': }
rbenv::build { '3.0.4': }
rbenv::build { '2.7.6': }
Copy link
Member Author

Choose a reason for hiding this comment

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

These are not the most up to date versions of Ruby but our choices are dictated by what's available from ruby-build.

Copy link
Member

Choose a reason for hiding this comment

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

"from ruby-build in EPEL", which we can influence, if we want.

Looking at https://canidepend.com/#ruby, we'd want 2.7.8, 3.0.4, 3.1.2 to match EL?


# rbenv
if $facts['os']['family'] == 'RedHat' and $facts['os']['release']['major'] != '7' {
include slave::rbenv
Copy link
Member Author

Choose a reason for hiding this comment

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

Intent is to add this to all our EL8 nodes, switch our code to it and then we can drop RVM and then update to EL9.

@ehelms ehelms marked this pull request as ready for review January 31, 2024 01:15
@ekohl ekohl linked an issue Feb 22, 2024 that may be closed by this pull request
@ekohl ekohl removed a link to an issue Feb 22, 2024
Comment on lines 170 to 172
include slave::rvm
if $facts['os']['family'] == 'RedHat' and $facts['os']['release']['major'] != '9' {
include slave::rvm
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
include slave::rvm
if $facts['os']['family'] == 'RedHat' and $facts['os']['release']['major'] != '9' {
include slave::rvm
}
if $facts['os']['family'] == 'RedHat' and $facts['os']['release']['major'] != '9' {
include slave::rvm
}

Otherwise you always include rvm ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Touche

# global => true,
# }
#
define rbenv::build (
Copy link
Member

Choose a reason for hiding this comment

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

I must admit I find the name of this define odd.
Yes, it's using ruby-build under the hood, but the rbenv command is rbenv install and to be even more precise, I'd probably not use the "action" (build/install) but the "object" (ruby) that we're managing here and call it rbenv::ruby.

That all said, this is based on the work in jdowning/rbenv and I have no idea how much we want to diverge from it, naming wise.

Copy link
Member Author

Choose a reason for hiding this comment

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

No real preference from me

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

Ack once tests are green

@@ -167,5 +167,12 @@
include slave::postgresql

# RVM
include slave::rvm
if $facts['os']['family'] == 'RedHat' and $facts['os']['release']['major'] != '9' {
Copy link
Member

Choose a reason for hiding this comment

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

This means Debian nodes don't get neither rvm nor rbenv, which is fine as we do not run unittests there, but needs tests fixed

@ehelms ehelms merged commit d429eb6 into theforeman:master Mar 19, 2024
2 checks passed
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.

3 participants