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

WIP, add pre/post copy and pre/post update tasks #1511

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hahuang65
Copy link

Hacking around for a potential solution to #240

Would love some feedback on whether we think this is a feasible solution or not.

If so, I need a little guidance on how to write some tests for the update tasks (they seem a bit difficult to test as we need to set up some Git repos), as well as how to test the difference between a pre and a post task.

Copy link
Member

@sisp sisp left a comment

Choose a reason for hiding this comment

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

Thanks for taking the initiative, @hahuang65! 🙏

Just a design feedback to start with: Having new top-level keywords _{pre,post}_{copy,update} looses the context that those are actually task definitions. How about extending the _tasks syntax similar to #326 and #1510?

_tasks:
  # implicitly a post-copy task
  - touch hello.txt
  # explicitly a pre-copy task using Jinja
  - "{% if _stage == 'before_copy' %}touch hello.txt{% endif %}"
  # uses environment variable `STAGE={before,after}_{copy,update}`
  - ./task.sh
  # new convenient syntax (#326, #1510)
  - command: touch hello.txt
    when: "{{ _stage == 'before' }}"

This would be backwards compatible when using the old syntax and add convenience for conditional tasks when using the new syntax.

@hahuang65
Copy link
Author

thanks for the feedback @sisp!

Does _tasks already support templated code in it? To me, adding that may make the yaml file have less sections, but certainly adds complexity to the existing sections. More to maintain and more to test. I don't believe the syntax to be quite readable too.

It also makes it much more difficult to simply port an existing shell script into the _tasks section.

Those are just my thoughts. I'm happy to oblige with your design suggestion, especially if it's already been pre-established.

@sisp
Copy link
Member

sisp commented Feb 13, 2024

@hahuang65 Yes, _tasks supports templating already; see its docs.

As it turns out, #1510 is already adding the new, more convenient syntax not only for migrations but also for tasks. It might make sense to wait for it to be merged and then build this PR upon it.

@sisp sisp mentioned this pull request Mar 11, 2024
@sisp
Copy link
Member

sisp commented May 15, 2024

@hahuang65 If you're interested in continuing work on this PR, you may now build upon the implementation of #1510 that we just merged.

@hahuang65
Copy link
Author

@sisp exciting! Unfortunately, probably for the next 2 to 3 months I will be busy. If someone would like to take over, feel free. Otherwise, I'll re-visit in a few months.

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.

2 participants