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

Allow template-path to be URL #66

Closed
vrom911 opened this issue Feb 8, 2021 · 13 comments
Closed

Allow template-path to be URL #66

vrom911 opened this issue Feb 8, 2021 · 13 comments
Assignees
Labels
feature New feature or request research Some research must be done before implementation
Milestone

Comments

@vrom911
Copy link

vrom911 commented Feb 8, 2021

I wonder if you think it could be possible to have something like this. My case is that lots of repositories have the same template we are using through our organization. It would be nice if we could have one template in the dedicated repo and in each particular project's .headroom.yaml we could just provide a link in there, instead of adding the templates folder in each project.

This is just an idea, I want to see what do you think about it and what options there are 🙂

@vaclavsvejcar
Copy link
Owner

This is pretty cool idea, thanks for suggesting it! 🙂 Thinking about the implementation, adding command line argument or YAML config option for referencing single URL template would be quite straightforward, but I'm not sure how to do something like referencing folder of templates (like you can do currently on local filesystem). The latter would likely require to add some more sophisticated support for template repositories, etc. What do you think? Or would the option to specify URL templates one by one be sufficient for now?

@vaclavsvejcar vaclavsvejcar added the feature New feature or request label Feb 8, 2021
@vrom911
Copy link
Author

vrom911 commented Feb 9, 2021

Yes, I think this would be awesome to have with separate file URLs at the moment. I completely agree that it is too complicated with folder, but the separate file URL in config/CLI would be fantastic for general use-case!

@vaclavsvejcar
Copy link
Owner

Awesome, let's make this happen in upcoming v0.4.1.0 🙂

@vaclavsvejcar vaclavsvejcar self-assigned this Feb 9, 2021
@vaclavsvejcar vaclavsvejcar added this to the v0.4.1.0 milestone Feb 9, 2021
@vaclavsvejcar vaclavsvejcar added the research Some research must be done before implementation label Mar 11, 2021
@vaclavsvejcar
Copy link
Owner

vaclavsvejcar commented Mar 12, 2021

So I started to think about this one and at least for now, there are some open questions that will need to be decided before actually implementing this. I'm mainly just thinking aloud here but maybe @vrom911 you'll also have some ideas about it 🙂

First, for each template file, Headroom needs to know for which type of source code file this template is and what type of template engine to use. In locally stored templates this is specified in template name as <FILE_TYPE>.<TEMPLATE_TYPE_EXT>, such as folder/haskell.mustache. Question is how to do this for URL located templates. I guess I can just require that the URL contains this information as well, so Mustache template for Haskell would be available for example as https://example.com/path/haskell.mustache, but I'm not sure if it isn't too much restricting. Otherwise it would be necessary to specify this info together with the URL in cmdline parameter or in YAML config file.

Second, I'll really need to rethink how to handle precedence of templates if you define same template from multiple sources. Including this change, Headroom will allow to use 3 different sources of templates. First, built-in templates available under --builtin-templates=<LICENSE> cmdline argument, second are templates stored in template folder and third will be template fetched from URL. When it happens that let's say two Mustache templates for Haskell will be defined at the same time, Headroom will somehow need to decide how to handle this. One option is to just reject such state, but it prevent use cases when you want to use let's say built-in templates and override only single one by your custom template either from local file or URL. Another option is to define override strategy, but I don't want to decide this for user. I'd rather make this configurable with default configuration that would allow overriding in direction like built-in templates -> local templates -> URL templates and if that doesn't fit the end user, she/he can just change it using some configuration such as something like:

template-priority:
  built-in: 1
  local: 2
  url: 3

What do you think about these points? Would such solution also match your needs?

EDIT: Moving this issue to v0.4.2.0 milestone as I need to do another minor release first.

@vaclavsvejcar vaclavsvejcar modified the milestones: v0.4.1.0, v0.4.2.0 Mar 14, 2021
@chshersh
Copy link

Hey @vaclavsvejcar! Your proposal looks good to me 👍🏻 Can't wait to use this feature in action 🙂

I have slightly different thoughts about the design. I'm not sure that the benefit of user-specified flexibility worth the implementation cost. I think the built-in functionality should have the lowest priority. You can't change its behaviour and it's always available to you. Also, I think that the local file priority should be higher than the url one in case you want to download the template as a cache and avoid downloading. Basically, the reasoning is "the closer the source to you and the more control over it you have, the higher priority it should have".

As a side note, I'm thinking about native support for Dhall in headroom. The Dhall support definitely deserves a separate issue and thorough consideration of all trade-offs. But I'm mentioning it here because Dhall natively supports referring to resources by URL. And with Dhall we can reduce the .headroom.yaml config itself in all our repositories to 2 lines of reusable Dhall function call, which would be ideal for our use case.

These are just a few thoughts from my side, and I'm not insisting on any particular implementation 🙂 headroom already helps us a lot in maintaining module headers! 🔝

@vaclavsvejcar
Copy link
Owner

vaclavsvejcar commented Mar 16, 2021

Hello @chshersh, thank you for yout thoughts.

I have slightly different thoughts about the design. I'm not sure that the benefit of user-specified flexibility worth the implementation cost. I think the built-in functionality should have the lowest priority. You can't change its behaviour and it's always available to you. Also, I think that the local file priority should be higher than the url one in case you want to download the template as a cache and avoid downloading. Basically, the reasoning is "the closer the source to you and the more control over it you have, the higher priority it should have".

Good point. Regarding the ability to configure the behaviour - I guess I'll just skip this part and implement it if really needed in future. And regarding the priority - I like the idea that "the closer the source to you and the more control over it you have, the higher priority it should have", so let's have it that way 🙂

As a side note, I'm thinking about native support for Dhall in headroom. The Dhall support definitely deserves a separate issue and thorough consideration of all trade-offs. But I'm mentioning it here because Dhall natively supports referring to resources by URL. And with Dhall we can reduce the .headroom.yaml config itself in all our repositories to 2 lines of reusable Dhall function call, which would be ideal for our use case.

I know about Dhall, but have zero experience with it. When I was writing Headroom, I choose YAML because just everyone knows it. That's also the reason why I probably wouldn't drop YAML support, but eventually just add Dhall as second way how to configure Headroom. Anyway, I'll check it out and I created follow-up ticket so we can discuss it there 🙂

@chshersh
Copy link

@vaclavsvejcar I totally support your idea of not dropping the first choice of plain config option and providing Dhall only as an additional bonus. I have some experience with Dhall: we generate HLint warnings for relude and we use it at work. If that's okay with you, I can provide some thoughts on how this possibly could look and work in headroom.

@vaclavsvejcar
Copy link
Owner

@vaclavsvejcar I totally support your idea of not dropping the first choice of plain config option and providing Dhall only as an additional bonus. I have some experience with Dhall: we generate HLint warnings for relude and we use it at work. If that's okay with you, I can provide some thoughts on how this possibly could look and work in headroom.

@chshersh That would be awesome, I'd appreciate any experience with Dhall 🙂 I'll also give it a try right after I'll finish this one with URL-based templates.

vaclavsvejcar added a commit that referenced this issue Mar 31, 2021
vaclavsvejcar added a commit that referenced this issue Apr 8, 2021
vaclavsvejcar added a commit that referenced this issue Apr 15, 2021
@vaclavsvejcar
Copy link
Owner

Hello @vrom911 @chshersh, I implemented this change and it's merged in master, so you can expect in next Headroom release. I still need to do some testing to make sure it's production ready, but you can already test it and I'll be grateful for any feedback 🙂

In order to use it, just place the HTTP/HTTPS address where you normally put the local template path, i.e. either as command line option -t|--template-path https://foo.bar/haskell.mustache or as YAML configuration key

template-paths:
  - some/local/rust.mustache
  - https://foo.bar/haskell.mustache

@vaclavsvejcar
Copy link
Owner

@chshersh @vrom911 Hello, not sure if you got previous notification, just please when you'll have some time to take a quick look, let me know if current implementation suits you so I can eventually release it. Thanks a lot 🙂

@chshersh
Copy link

Hi @vaclavsvejcar! Indeed, the previous notification somehow was unnoticed. We've tested the latest version of headroom and introduced our templates in our org repository:

We tested the configuration and everything is working as expected 👍🏻 Thanks a lot for implementing this feature, it's very helpful!

@vaclavsvejcar
Copy link
Owner

@chshersh Awesome, happy to hear that! I'll then publish this in few days as part of the v0.4.2.0 release.

@vaclavsvejcar
Copy link
Owner

Released as part of v0.4.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request research Some research must be done before implementation
Projects
None yet
Development

No branches or pull requests

3 participants