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 #37495 - unify sending the built state #10179

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

sbernhard
Copy link
Contributor

No description provided.

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

From macros/docs perspective seems fine to me, just an idea inline:

app/services/foreman/renderer/scope/macros/base.rb Outdated Show resolved Hide resolved
@sbernhard sbernhard force-pushed the fix_37495 branch 2 times, most recently from 0090781 to faf14ef Compare May 31, 2024 15:11
@ekohl
Copy link
Member

ekohl commented May 31, 2024

I don't know if this is a bashism or POSIX shell, but you can indent EOF:

#!/bin/sh
if true ; then
	cat <<-EOF
	Hello
	World!
	EOF
fi

But you must use tabs, not spaces.

@sbernhard
Copy link
Contributor Author

But you must use tabs, not spaces.

And the indent() routine uses spaces :-)

@sbernhard
Copy link
Contributor Author

@ofedoren can we merge this? it is something we require to finish #9808

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Since there are changes in the templates, should we update the snapshots?

Otherwise LGTM, though I'd like final approval from someone else, maybe @stejskalleos

app/services/foreman/renderer/scope/macros/base.rb Outdated Show resolved Hide resolved
@sbernhard
Copy link
Contributor Author

Since there are changes in the templates, should we update the snapshots?

If you would know which templates I need to change, I would simply do this. But currently I see no template test which fails. Do we need to activate something?

@ofedoren
Copy link
Member

Since there are changes in the templates, should we update the snapshots?

If you would know which templates I need to change, I would simply do this. But currently I see no template test which fails. Do we need to activate something?

Most of the time whenever we change templates, we run RAILS_ENV=test bundle exec rake snapshots:generate to update the rendered version of a template, but I just run it locally and there were no changes, which is... weird I guess. I'd expect at least indentation changes...

@sbernhard
Copy link
Contributor Author

All tests are green @ofedoren

@ofedoren ofedoren merged commit e49650a into theforeman:develop Jun 26, 2024
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants