I like what @ekohl says. That means no need to copy to Foreman codebase anymore. We need to update seed script to load it from gem instead of local directory. Given the number of 
s, I think this is a popular option. Is this something we aim for 1.23? Who will send actual PRs? 
We’ll probably need some boilerplate code in the community templates PR. Can we also create a gem for that code so users can easily consume that in their custom template repo? So in the end we’ll have two gems:
community_templates - our content
foreman_templates_boilerplate - the boilerplate
Core will then require community_templates, community_templates will require foreman_templates_boilerplate.
Cant we just turn community-templates into a gem? It looks like a clean git repo - just few of directories. Or is there a different reason?
I know I will regret this. I can probably work on this.
1 Like
Now thinking about this more I see a possible problem with this. If we keep template snapshot test suite in core this would create unnecessary burden of breaking these tests everytime we do a change in community-templates (therefore jenkins will run against master branch gem repo).
I think that snapshot tests must be in the same repo as the source templates so we can review the changes (and snapshots) in advance prior merging a change in community-templates repo.
Which brings me to the idea which is exactly other way around - how about moving template snapshots to community repo and then run the snapshots test via foreman checkout similarly like we run plugins.
Edit: Or we can keep the idea but simply move the snapshot files to the community-templates, the test code would still be kept in Foreman core.
It feels like we should revive this discussion.
With the branching of 1.24 we again hit several issues due to changes in the community templates repo that could have been caught much sooner had we gone with one of the suggested solutions.
While all of the options could improve this, I think one option stands out as the simplest to implement, and I think this simplicity make it the options we should choose:
Lets archive the community templates repo and move to modifing the templates in the main foreman repo directly.
Pros:
- Simple to implement - can be done in a few minutes. No need to work on integrating gems, modifying tests etc.
- Allows testing all changes properly when a PR is opened. with @lzap’s work on improving the snapshot renderer in https://github.com/theforeman/foreman/pull/7140 we will be able to catch many more issues that we hit in the past.
- Changes in core that affect template rendering will also get tested with the latest templates, and allow adjusting the templates if needed in the same PR.
- Changes to snapshots can be done in the same PR as the changes to the template.
- We can easily track which changes are in each Foreman release (and trace where a bug landed if necessary for backports)
- No more need to sync templates during branching and release process.
Cons:
- PRs will require redmine issues. The concern has been raised that this may deter drive-by contributors, but I don’t think it will be a significant factor. It may also have a benefit of having more contributors take part in the foreman core after having a template PR merged. I looked at the logs for the 1.24 templates, and of 36 commits only 4 came from people who aren’t regular foreman contributors and didn’t include an issue number. In 1.23 there was only one such commit.
I think the pros greatly outweigh the con here - and I don’t think requiring a redmine issue is such a high bar that it will actually have a significant effect on contribution.
- Yes, let’s archive community templates and start doing template changes in core
- No, let’s keep community templates active and figure out a better way to test and integrate it
0 voters
Another con is that we’d increase the load on our CI.
I’d still like to see community-templates as a proper gem that we can include in Foreman with proper CI, including a nightly/weekly build against Foreman as we do for all plugins. We have a helper rake task (that may need some love) which can help with this. We can look at creating a nightly gem out of this (we do it for Katello already).
Getting things merged in Core Foreman is hard enough. Let’s not add more.
You mean due to ~2-3 additional PRs per week? I don’t think that’s a significant difference.
This also brings to mind another pro i didn’t think of earlier - more visibility into PRs. More people look at core than community templates, and the people who do look at community templates today will have one less repo to look at.
Could you elaborate on what are the pros and cons of this option?
In this case the templates are already merged in core - and we manually sync them all the time. There is no effort needed to get the templates in core as they are already there, and we would save the effort put into syncing them and fixing snapshots every time.
Other than the templates themselves the only code in the repo is a few tests, which I believe have much lower coverage than the tests we have in core (as is evident by the bugs discovered when syncing the templates). We could also migrate these if we want to with very little effort.
I like moving the templates to core. We’ve been talking about packaging templates separately for a long time and I’m afraid, we’d just keep talking. In fact I see a benefit of more reviewers, better visibility, better connection of the change e.g. when we also need to enable something in safemode. Changes propagate to nightly also much faster. Seems like a quick win not preventing us packaging templates separately if we ever get to it.
It’s not uncommon that people open PR with templates changes against core and we are telling them to send the PR to community-templates repo. Meaning, people expect it in the main repo.
IIRC the main reason for keeping this outside of the main repo was the versioning. We kept compatibility with Foreman stable and 2 previous versions and we didn’t version templates at all. That is no longer the case for some time.
1 Like
To illustrate the urgency on another example, how the late sync causes problems reported too late - Bug #28156: db seed failure: ArgumentError: Malformed version number string >= 3.1.0 - Foreman, was reported on nightly but is a 1.24 blocker that we could have known 2 weeks ago, when the template was merged. We rarely sync templates during the 3 months dev phase. We should resolve this soon.
1 Like
I believe this has a simple solution - let’s create a CI job in community-templates:
- Checkout Foreman develop (or stable for stable branches)
- Run a rake task to render all templates:
bundle exec rake templates:render DIRECTORY=/xyz/community-templates/provisioning_templates/ - Profit
Slightly improved version:
- Improve our snapshot test and rake task so both snapshots and templates directories can be configured in a similar manner
- Create a job to perform such test
- Keep copies of snapshots in community-templates repo as files in git so we can easily do reviews
I think this can be implemented much quicker than extracting renderer into a gem and using that as a dependency in community-templates.
Won’t the CI job fail every time the snapshot changes?
This would mean having to do an additional sync from core to templates to make sure the snapshots match what is in templates.
What would be the benefit of this approach over merging the repos?
What I propose is to have templates and snapshots in both community-templates and in foreman core repo (a copy).
Also to illustrate that having a separate repo confuses contributors, here are a few PRs opened against core with template changes that we had to ask to move to community templates:
https://github.com/theforeman/foreman/pull/6798
https://github.com/theforeman/foreman/pull/6514
https://github.com/theforeman/foreman/pull/5746
https://github.com/theforeman/foreman/pull/5766
how does that solve the problem of having to manually make sure the repos are in sync all the time?
I am not going to re-read the whole thread, but this was created as “testing & separate repository” so I am trying to figure out the best approach for tests. If this is your major concern too, than I think we should ship community-templates as a gem dependency and remove copies in core git. That’s what’s best for users. Or to merge everything into single core git, that’s what’s best for developers.
My proposal when bringing this thread back after 6 months of inactivity, following the issues we hit during this branching and release cycle, was to merge it into core.
I still believe this is the simplest solution and am still not convinced there is any significant benefit to be gained from the effort required to extract it into a separate gem.
We still want to have snapshots in foreman core to make sure changes in core code don’t break template rendering, and that means we would need to update the snapshots in two repos for every change in the templates.
I agree however I can feel the pain for users. Recently, I had to checkout 2GB linux kernel git just to update few lines in the documentation/ directory.
As I mentioned above, some of the users already do submit prs directly to foreman only to be told to go to community templates. Foreman is just about 100MB repo, and shouldn’t take more than a few minutes to clone even on a slow connection. Some small contributions can even be done using the github UI.
I’ve changed my vote to YES. I think benefits are above cons.
2 Likes