-
Notifications
You must be signed in to change notification settings - Fork 93
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
Added Jinja templating preprocessing engine as well as unit test fixes when running on Windows. #104
base: master
Are you sure you want to change the base?
Conversation
Added normalizePath() call in test method to change '\' to '/' if running on windows.
This is @tomzo's call. But from the surface, this looks good to me. After @tomzo approves, would you mind updating the readme with pointers to a few example tests showing some of the features you mention? I'll setup something to do dependency checks on transitive dependencies. To make sure that the dependencies are not using a copyleft license, vulnerabilities etc. |
Will do. I am definitely working on more comprehensive examples. It may take a couple of days. |
No hurries, take your time. This will go in the next release of GoCD, due
in another month or so.
- Ketan
…On Tue, Jan 29, 2019 at 8:57 AM PredatorVI ***@***.***> wrote:
Will do. I am definitely working on more comprehensive examples. It may
take a couple of days.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#104 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAApZttBWU8E82X5Q_1t9zXapVAfcRSaks5vH7-ZgaJpZM4aXArg>
.
|
Thanks, this is a great feature. Just a thought, maybe it would be better to have files meant for template preprocessing end with |
I wondered about a separate extension, but opted not to based on my experience on how YAML is used in SaltStack or Ansible, or similarly with how a C/C++ compiler will preprocess .c/.cpp files. In both cases, there is no distinction between files containing template directives vs files without template directives. |
Dear all, we have successfully applied your much appreciated pull request to our build environment. The plugin does not yet support the definition of templates that pipelines can reference to. But we failed to make use of Jinja's include statement.
To solve the "issue" we would like to ask you for the following addition to your work: In YamlConfigParser you are creating an instance of Jinjava. The reason is: Jinjava uses the ClasspathResourceLocator. Jinjava jinjava = new Jinjava(); What do you think of the proposal? Thanks |
@ABETES (et. al.), I'm a little slow getting examples put together but I sort of expected needing a custom resource locator. Thanks for working through that. It makes sense to use the 'baseDir' as the root context from parseFiles(). This would limit the inclusion scope to a per repo "namespace" and feels like the right way. My question would be how to handle the cases where parseStream() is called directly (from YamlPluginCLI)? I've not used YamlPluginCli so I don't know what the use cases are and what baseDir should be? Current working directory? Parent directory of 'syntax.file'? Thoughts? |
@PredatorVI Good question. I'd say, for now set it to current working directory. Once @marques-work is back from vacation, we might need to rethink the files being sent over from the GoCD server side. I'm not sure if the entire directory is sent to |
@PredatorVI Hi there, thanks for the contribution of such a nice feature! I wanted to shed some light on how Most likely, the main use case is probably going to be in the Anyhow, you're probably right that we'd need some kind of custom resource locator for this to work in the above case. I haven't looked at Jinjava, but I'm hoping it's not difficult to create one for this use case. One more restriction to think about here with regards to Regarding file extensions, I would prefer that the template files have distinct extensions as @tomzo suggested. It's theoretically possible for files to have contents that look like tokens, but aren't intended to be. Perhaps an argument to an |
Totally agree with all that. Let's be nice and declare what is supposed to be expanded and what is not. Moreover, after considering the templates fragments used in jinja
@marques-work does that sound right to you?
@marques-work I think, we should recommend the clients to actually include the full path then. Otherwise there is no way for statements like this I'd like to get this feature going, since it has stalled a bit. Here is how I see getting this feature further:
Then we can work on resource files in next PRs. @PredatorVI @marques-work Sounds good? |
@tomzo yes, there are potentially 3 types of files that you enumerated that we treat differently.
I was actually referrring to HTTP clients (like
@PredatorVI @tomzo this will probably mean that users need to put all files in the same directory as templates if they want to use I agree that we can integrate this first without support for external |
@marques-work Might be worth checking if this works:
|
@arvindsv I will try that but I suspect that will create another boundary with field name |
@arvindsv it's always nice to be wrong too :)
So I suppose we could support hierarchies by putting the burden on the user to override the filenames for each file they post for the API. All the more reason to favor the CLI tool IMO since we could take care of this automatically there. We'll also need some careful documentation, but at least we know there is a workaround involving a minimal amount of extra typing. |
I've added the Jinjava Jinja pre-processing engine to the plugin to allow for embedded Jinja templates.
I'm not yet completely aware of what limitations might exist in Jinjava compared to official Jinja/Jinja2 implementations.
The unit test is minimal (creates 3 pipelines in a loop) but I can add more complex examples over time as needed. The hope is to be able to reduce complexity and duplication on an even larger scale than currently allowed, including (but not limited to):
Feedback appreciated.