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

Add ability to define templates library at module level #2332

Merged
merged 14 commits into from
Sep 21, 2021

Conversation

pditommaso
Copy link
Member

This PR replaces #2263 adding some minor refactoring and unit testing.

Signed-off-by: Manuele Simi <[email protected]>
Signed-off-by: Manuele Simi <[email protected]>
@manuelesimi
Copy link
Collaborator

I added a Module Templates section to the DSL2 doc.

I noticed how you tested the behavior of TaskContext with the new template location. Brilliant, congrats! I realize I didn't have enough knowledge of NF to write something like that.... For one, I was forgetting to enable DSL2 in my tests:)

Copy link
Member Author

@pditommaso pditommaso left a comment

Choose a reason for hiding this comment

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

Thinks looks great. Just made a comment on file template existence check.

@abhi18av
Copy link
Member

I'm just curious here, the approach used in this PR , could also be applied for module level bin folder as well right ?

#1798

@manuelesimi
Copy link
Collaborator

@abhi18av The rational behind #1798 is indeed the same of this PR. They both aim to achieve self-contained module components. However, the solution found in this PR does not apply for the bin folder.
The templates dir is resolved locally, before the task is submitted to the execution layer. On the other hand, the bin dir must be uploaded on the node where the task is executed. Another difference is that there is only one template file per task, while multiple executable scripts can be both in the module's and workflow's bin folder.

That said, solving #1798 would nicely pair with the solution we got here.

@manuelesimi manuelesimi marked this pull request as draft September 20, 2021 14:32
@manuelesimi manuelesimi marked this pull request as ready for review September 20, 2021 14:42
@abhi18av
Copy link
Member

On the other hand, the bin dir must be uploaded on the node where the task is executed.

Ah, I see!

Thanks for the detailed explanation @manuelesimi!

@pditommaso
Copy link
Member Author

Ok, think we can merge this. Nice contribution @manuelesimi, thanks!

@pditommaso pditommaso merged commit 7e7d525 into master Sep 21, 2021
@pditommaso pditommaso deleted the manuelesimi-feature/loadTemplateFromModuleDir branch September 21, 2021 07:52
@manuelesimi
Copy link
Collaborator

Ok, think we can merge this. Nice contribution @manuelesimi, thanks!

You're welcome!

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

Successfully merging this pull request may close these issues.

3 participants