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 #37433 - Set indentation when calling subscription_manager_setup #10153

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

jpasqualetto
Copy link
Contributor

Adding proper indentation when calling snippet subscription_manager_setup

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.

I'd argue the rest of the subman code should be not indented since at the bash level they're not conditional. The Ruby conditionals shouldn't matter for the end result.

@jpasqualetto
Copy link
Contributor Author

jpasqualetto commented May 10, 2024

The problem I'm trying to solve is the final rendered result of cloud_init_default.erb.

subscription_manager_setup.erb is fine.

redhat_register.erb is also fine and generates valid bash script.

However, when rendered, redhat_register.erb result does not have the same indentation. Code that comes from redhat_register.rb have 2 space indentation, but what is generated by the snippet has no indentation at all. That's fine if that result is simply being processed by bash.

The problem is that cloud_init_default.erb (which consumes redhat_register.erb) is supposed to generate a valid yaml as result. With that difference on indentation coming from redhat_register.erb its result is not a valid yaml.

I wouldn't mind if we removed all the indentation from redhat_register.erb. That would do the trick too. What matters is that the rendered result should have the same indentation.

@ShimShtein
Copy link
Member

@ekohl , this is how the template looks like when it's rendered without the indentation:
image

If we indent the internal snippet, it will pass the validation:
image

@stejskalleos
Copy link
Contributor

@jpasqualetto can you update the snapshots of templates as well?

The rails command is RAILS_ENV=test bundle exec rails snapshots:generate

@stejskalleos stejskalleos self-assigned this Jun 5, 2024
@jpasqualetto jpasqualetto requested a review from a team as a code owner July 23, 2024 20:21
@github-actions github-actions bot added UI Legacy JS PRs making changes in the legacy Javascript stack labels Jul 23, 2024
@jpasqualetto
Copy link
Contributor Author

I messed up while trying to push the snapshots. I rebased my code to run the generate snapshots and pushed it into the issue branch. Trying to fix this mess now.

@jpasqualetto
Copy link
Contributor Author

I added the snapshots and fixed my mess. Let me know if you need anything else.

Copy link
Member

@ShimShtein ShimShtein left a comment

Choose a reason for hiding this comment

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

LGTM. Looks pretty neat to me. @ekohl any comments?

@@ -96,7 +96,7 @@ description: |
echo "Starting the subscription-manager registration process"

# Set up subscription-manager
<%= snippet("subscription_manager_setup", variables: { subman_setup_scenario: 'provisioning' }).strip -%>
<%= indent(2) { snippet("subscription_manager_setup", variables: { subman_setup_scenario: 'provisioning' }).strip } -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, skip1 is necessary so that the first line (# select...) is indented correctly.

@ekohl ekohl merged commit aa8f66f into theforeman:develop Jul 29, 2024
49 of 53 checks passed
@ShimShtein
Copy link
Member

@ekohl Apparently the indentation breaks templates that have heredoc in them. Created a ticket for it:
https://projects.theforeman.org/issues/37741

@ekohl
Copy link
Member

ekohl commented Aug 19, 2024

Given my concern in #10153 (review) I'm leaning to reverting the change. Or are you working on a fix @ShimShtein?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Legacy JS PRs making changes in the legacy Javascript stack Templates UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants