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

Improve support of ruby versions and features #30

Merged
merged 6 commits into from
Nov 22, 2024
Merged

Conversation

lloeki
Copy link
Contributor

@lloeki lloeki commented Nov 21, 2024

No description provided.

@lloeki lloeki force-pushed the lloeki/improve-support branch 12 times, most recently from 79c13c1 to c03199a Compare November 21, 2024 12:56
##
# Use TestTask.create instead.

def initialize name = :test # :nodoc:

Choose a reason for hiding this comment

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

Code Quality Violation

Suggested change
def initialize name = :test # :nodoc:
def initialize(name = :test) # :nodoc:
Use parentheses when the method receives arguments (...read more)

In Ruby, parentheses are not required when defining methods without arguments. The rule "Avoid parentheses for methods without arguments" encourages this practice, making the code cleaner and more readable.

This rule is crucial because it promotes consistency and clarity in your code. Ruby is known for its elegant and human-readable syntax, and following this rule maintains that reputation. Using parentheses for methods without arguments can cause unnecessary confusion and clutter in your code.

To adhere to this rule, omit parentheses when defining methods without arguments. For instance, instead of def method(), use def method. For methods with arguments, continue using parentheses to separate the method name from its arguments, like def method(arg1, arg2). Following this rule will make your Ruby code cleaner and easier to read.

View in Datadog  Leave us feedback  Documentation

# Create several test-oriented tasks under +name+. Takes an
# optional block to customize variables.

def self.create name = :test, &block

Choose a reason for hiding this comment

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

Code Quality Violation

Suggested change
def self.create name = :test, &block
def self.create(name = :test, &block)
Use parentheses when the method receives arguments (...read more)

In Ruby, parentheses are not required when defining methods without arguments. The rule "Avoid parentheses for methods without arguments" encourages this practice, making the code cleaner and more readable.

This rule is crucial because it promotes consistency and clarity in your code. Ruby is known for its elegant and human-readable syntax, and following this rule maintains that reputation. Using parentheses for methods without arguments can cause unnecessary confusion and clutter in your code.

To adhere to this rule, omit parentheses when defining methods without arguments. For instance, instead of def method(), use def method. For methods with arguments, continue using parentheses to separate the method name from its arguments, like def method(arg1, arg2). Following this rule will make your Ruby code cleaner and easier to read.

View in Datadog  Leave us feedback  Documentation

Comment on lines +114 to +115
self.test_globs = ["test/**/test_*.rb",
"test/**/*_test.rb"]

Choose a reason for hiding this comment

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

Code Quality Violation

Consider using the %W syntax instead (...read more)

The rule "Prefer %w to the literal array syntax" is a Ruby style guideline that encourages the use of %w notation instead of the traditional array syntax when defining arrays of strings. This rule is part of the Ruby community's efforts to promote readability and simplicity in Ruby code.

This rule is important because it helps to keep the code concise and easy to read. The %w notation allows you to define an array of strings without having to use quotes and commas. This can make the code cleaner and easier to understand, especially when dealing with large arrays.

To follow this rule, replace the traditional array syntax with the %w notation. For example, instead of writing ['foo', 'bar', 'baz'], you should write %w[foo bar baz]. This will create the same array, but in a more readable and concise way. By following this rule, you can help to make your Ruby code cleaner and easier to understand.

View in Datadog  Leave us feedback  Documentation

##
# Generate the test command-line.

def make_test_cmd globs = test_globs

Choose a reason for hiding this comment

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

Code Quality Violation

Suggested change
def make_test_cmd globs = test_globs
def make_test_cmd(globs = test_globs)
Use parentheses when the method receives arguments (...read more)

In Ruby, parentheses are not required when defining methods without arguments. The rule "Avoid parentheses for methods without arguments" encourages this practice, making the code cleaner and more readable.

This rule is crucial because it promotes consistency and clarity in your code. Ruby is known for its elegant and human-readable syntax, and following this rule maintains that reputation. Using parentheses for methods without arguments can cause unnecessary confusion and clutter in your code.

To adhere to this rule, omit parentheses when defining methods without arguments. For instance, instead of def method(), use def method. For methods with arguments, continue using parentheses to separate the method name from its arguments, like def method(arg1, arg2). Following this rule will make your Ruby code cleaner and easier to read.

View in Datadog  Leave us feedback  Documentation

@lloeki lloeki force-pushed the lloeki/improve-support branch 5 times, most recently from f7f404f to 6ce587e Compare November 21, 2024 14:01
@lloeki lloeki marked this pull request as ready for review November 21, 2024 14:33
@lloeki lloeki requested a review from a team as a code owner November 21, 2024 14:33
Copy link

@delner delner left a comment

Choose a reason for hiding this comment

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

A tremendous amount of code here, particularly in the various images. For the sake of learning, and knowledge for our future selves, can you share some of the essential "need-to-know" elements that made these images work? (In broad strokes?)

It'd also be nice to call out some of the challenges you overcame, potential risks, brittle parts we might need to be careful with rolling forward.

@@ -3,4 +3,8 @@
# @type self: Rake::TaskLib

# load rake tasks from tasks directory
Dir.glob(File.join(__dir__ || Dir.pwd, "tasks", "**", "*.rake")) { |f| import f }
if RUBY_VERSION.start_with?("1.8.")
Copy link

Choose a reason for hiding this comment

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

1.8 didn't handle globs? Or is there legitimately only one file to load?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the regular line loads all rake tasks in the tasks directory, some of which are unparseable in Ruby 1.8 (mostly because of foo: bar 1.9-style hashes), and we only need the test.rake one anyway in that case.

@@ -0,0 +1,253 @@
# platforms: linux/x86_64

# CentOS 7.9 has glibc 2.17
Copy link

Choose a reason for hiding this comment

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

Just out of curiosity: how much of this had to be written from scratch? Vs say copying from another Dockerfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handwritten.

I mean, I started with 2.7, iteratively worked my way up to 3.4 (which meant adding Rust), and then iteratively down to 1.8.7 (which meant solving an increasing number of legacy issues), then some cleanups and unification.

There's not much to it really, it's very repetitive gruntwork. Some bits were initially copied from the "usual way" Ruby is built from source on Debian&al but that was to save a bit of time since it's essentially curl+configure+make+make install, with some adjustments to cater for RedHat&al.

@lloeki
Copy link
Contributor Author

lloeki commented Nov 21, 2024

For the sake of learning, and knowledge for our future selves, can you share some of the essential "need-to-know" elements that made these images work? (In broad strokes?)

It'd also be nice to call out some of the challenges you overcame, potential risks, brittle parts we might need to be careful with rolling forward.

Regarding to old Ruby specifically? Or this image building in general?

Regarding the former, I'll go back to the whole of it at some point (for one thing the whole thing is not quite optimal in size), do some cleanups, and add a bit more comments.

Regarding the latter, I'll expand the README (but not too much) and another more detailed file about how this all works, notably how the caching is leveraged to maximise cross-image layer reuse and minimise size on platter and on wire.

But right now this (and its dependent) needs to work, as well as other unrelated priority tasks.

@TonyCTHsu
Copy link
Contributor

I approved, but I am a bit hesitate to embed so much legacy Ruby knowledge into code.

@lloeki lloeki merged commit 677a9b5 into main Nov 22, 2024
46 checks passed
@lloeki lloeki deleted the lloeki/improve-support branch November 22, 2024 07:29
@lloeki
Copy link
Contributor Author

lloeki commented Nov 22, 2024

I am a bit hesitate to embed so much legacy Ruby knowledge into code.

@TonyCTHsu How would you rather have it be? Documentation (be it detailed comments or some README)? Is there any particular area you feel uncomfortable about?

Feel free to start line-bound comments on this PR, so that I can assuage your uneasiness.

@TonyCTHsu
Copy link
Contributor

How would you rather have it be?

I would like to treat those legacy stuff as exception, which we might only going to publish it once for exceptional situation.

That means, having them as a separate namespace, separate workflows. Isolated them from our usual process. For example, when I added a Ruby image 3.5 next year. I don't want to be concerned about Ruby 1.8 for CentOS.

@lloeki
Copy link
Contributor Author

lloeki commented Nov 26, 2024

I beg to differ:

  • If it starts failing it means it may be failing for customers as well, we need to be aware of that.
  • The old rubies being unmoving targets the only variable is the base OS or the CI environment; if we start having trouble with that we're having trouble across all versions; also the base OS is CentOS, which is extremely stable as well.
  • If old files don't change, due to layer caching no image will change (so no impact) ; conversely if something changes we want it validated.
  • Introducing a special case means the special case is... special and thus not part of the usual process, which opens rooms for mistakes and ironically would need additional maintenance. Due to layer caching the cost of running is very low (by design), so keeping them as a generic case costs very little, to great benefit.

For example, when I added a Ruby image 3.5 next year. I don't want to be concerned about Ruby 1.8 for CentOS.

In such a purported PR, since nothing about Ruby 1.8 will have change how would adding 3.5 impact anything? With layer caching 1.8 will most probably very well be the exact same image layers down to the last bit as it is today without special casing anything.

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