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

Fixes #37792 - Safe mode and bootdisk_* template helpers #167

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

stejskalleos
Copy link
Collaborator

Using :extend_template_helpers instead of :allowed_template_helpers
fix the issue when rendering bootdisk_* template helpers in the Safe mode.

For steps to reproduce, see https://projects.theforeman.org/issues/37792

Copy link
Contributor

@nofaralfasi nofaralfasi left a comment

Choose a reason for hiding this comment

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

Failing tests are related.

Using :extend_template_helpers instead of :allowed_template_helpers
fix the issue when rendering bootdisk_* template helpers in the Safe mode.
@stejskalleos
Copy link
Collaborator Author

Moving to draft, current changes breaks the full-host iso generation

@stejskalleos stejskalleos marked this pull request as draft September 12, 2024 12:27
@stejskalleos stejskalleos marked this pull request as draft September 12, 2024 12:27
@stejskalleos stejskalleos marked this pull request as ready for review September 17, 2024 08:21
@stejskalleos
Copy link
Collaborator Author

@nofaralfasi ready for review

tested subnet, host image and full host images and all generated correctly

@nofaralfasi
Copy link
Contributor

This fix resolved the issue for me, but I'm unclear about why this change was necessary. @stejskalleos, could you please explain the reason for these changes? I’m assuming it was working previously, so what led to this problem?

@stejskalleos
Copy link
Collaborator Author

@nofaralfasi I believe the reason is the recent move to Zeitwerk and its way of loading modules and classes.

With the original approach, the ForemanBootdisk::Scope::Module wasn't loaded before the Foreman's render method tried to access methods from it, causing the no method error.

Copy link
Contributor

@nofaralfasi nofaralfasi left a comment

Choose a reason for hiding this comment

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

I must admit that I still don’t fully understand some of these changes, such as the necessity of keeping the ForemanBootdisk::Scope::Bootdisk class. However, since this resolves the issue, I don't want to hold up the merge.

@stejskalleos
Copy link
Collaborator Author

stejskalleos commented Sep 27, 2024

Zeitwerk follows a strict file structure-to-module/class mapping.

For example, having this code:

module ForemanBootdisk
  module Templates
    module Scope
      # do something
    end
  end
end

It requires you to have the following files:

foreman_bootdisk.rb
foreman_bootdisk/templates.rb
foreman_bootdisk/templates/scope.rb

Zeitwerk maps namespaces (modules or classes) to the corresponding file paths. When a constant like ForemanBootdisk::Templates::Scope is referenced for the first time, Zeitwerk looks for the file templates/scope.rb within the foreman_bootdisk directory. If the modules are defined within the same file, Zeitwerk won't be able to autoload ForemanBootdisk::Templates::Scope correctly.

cc @ofedoren please feel free to correct me if I'm wrong.

@stejskalleos stejskalleos merged commit 415780d into theforeman:master Oct 1, 2024
22 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.

2 participants