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 #25613 - template renderer tests use os, family, loader #7140

Closed

Conversation

lzap
Copy link
Member

@lzap lzap commented Oct 30, 2019

This greatly enhances coverage of our tests.

@theforeman-bot
Copy link
Member

Issues: #25613

@lzap
Copy link
Member Author

lzap commented Oct 30, 2019

I think PXE loader is useless in this context, let me remove it.

@lzap lzap force-pushed the template-tests-os-family-loader-25613 branch from 80b3a67 to 90bea1a Compare October 30, 2019 10:20
@lzap lzap force-pushed the template-tests-os-family-loader-25613 branch from 90bea1a to 9ad7a5a Compare October 30, 2019 10:20
@lzap
Copy link
Member Author

lzap commented Oct 30, 2019

Rebased, added more OSes. Some templates are rendered with irrelevant context, but git will work fine with few more files and we want to cover as much as possible.

@lzap
Copy link
Member Author

lzap commented Oct 30, 2019

Test is not right, need to work on this more.

@lzap lzap force-pushed the template-tests-os-family-loader-25613 branch from 9ad7a5a to 4d64847 Compare October 30, 2019 11:27
@lzap lzap force-pushed the template-tests-os-family-loader-25613 branch from 4d64847 to 60aba3c Compare October 30, 2019 11:47
# global boot
-
name: PXELinux/pxelinux_global_default.erb
os: []
Copy link
Member

Choose a reason for hiding this comment

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

Can we get this from the template metadata?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well only operating system name and version. Not family and possibly other things. I'd rather be explicit. I am currently working on adding host params here.

Copy link
Member

Choose a reason for hiding this comment

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

Note, that this is something we'd need to keep in sync when importing community templates. I doubt this will actually work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I want specifically to be testing RHEL 6, 7, 8 and similarly Debian/Ubuntu. Let me think about it tomorrow.

@lzap lzap force-pushed the template-tests-os-family-loader-25613 branch from 60aba3c to 12a371e Compare October 30, 2019 12:42
@timogoebel
Copy link
Member

Note, that instead of changing the test implementation we'd better move the code to use

class RenderTemplatesFromFolder
, which has a better design.

@lzap
Copy link
Member Author

lzap commented Nov 20, 2019

Timo, do you think merging TemplateSnapshotService and RenderTemplatesFromFolder into one class? I don't understand why this was created in the first place, but I was one merging it so no excuses.

@timogoebel
Copy link
Member

Timo, do you think merging TemplateSnapshotService and RenderTemplatesFromFolder into one class?

Yep, that would be great. RenderTemplatesFromFolder has the better design and should be used as a default.

@theforeman-bot
Copy link
Member

Thank you for your contribution, @lzap! This PR has been inactive for 3 months, closing for now.
Feel free to reopen when you return to it. This is an automated process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants