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

Hooks #1449

Merged
merged 20 commits into from
Jan 31, 2025
Merged

Hooks #1449

merged 20 commits into from
Jan 31, 2025

Conversation

aljazerzen
Copy link
Contributor

@aljazerzen aljazerzen commented Jan 22, 2025

  • make hooks a single string, instead of an array,
  • invoke /bin/sh instead of the process directly,
  • set cwd,
  • WSL
  • tests

  • project.init.after
  • branch.switch.after
  • branch.wipe.after
  • migration.apply.after

Do we want before hooks too? I've implemented some because they were easy.

  • project.init.before
  • branch.switch.before
  • branch.wipe.before
  • migration.apply.before

@aljazerzen aljazerzen changed the title hooks Hooks Jan 22, 2025
@aljazerzen aljazerzen marked this pull request as draft January 22, 2025 18:09
@aljazerzen aljazerzen changed the base branch from master to project-manifest January 22, 2025 18:09
@aljazerzen aljazerzen force-pushed the hooks branch 2 times, most recently from d3deae9 to bea254b Compare January 23, 2025 17:15
Base automatically changed from project-manifest to master January 27, 2025 11:47
@aljazerzen aljazerzen marked this pull request as ready for review January 28, 2025 11:12
@aljazerzen
Copy link
Contributor Author

I need an install of windows to implement hooks in WSL.

Copy link
Member

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

Looks good!

src/branch/switch.rs Outdated Show resolved Hide resolved
src/portable/windows.rs Show resolved Hide resolved
tests/proj/project3/gel.toml Outdated Show resolved Hide resolved
@vpetrovykh
Copy link
Member

  • project.init.after hook works and triggers migration hooks if there are migrations
  • branch.switch.before and branch.switch.after hooks respond to switch, but they don't trigger migration hooks
  • branch.wipe.before and branch.wipe.after hooks respond to wipe, but they don't trigger migration hooks
  • rebase and merge branch commands don't trigger branch hooks and instead trigger migration hooks (which is correct)
  • I was able to chain simple commands directly in the gel.toml. I was able to see environment variables and run scripts in the root of the project directory as intended.

My original intent with the RFC was that any schema change to a consistent schema state should trigger migration hooks. That means all invocation of migrations should do it, but also any switches between branches (since each branch is in a consistent schema state, potentially different from previous branch). We can argue that the branch hooks can handle schema changes on their own and don't need to also trigger migration hooks. The original separation was to keep schema-related scripts connected to migration hooks and non-schema related config connected to branch and project init hooks. This kind of thing should follow some kind of intuition, although obviously the docs would disambiguate which hooks get triggered by which commands.

@vpetrovykh
Copy link
Member

There's a hook behavior that was not specified in the RFC one way or another. When a hook fails, the operation aborts at that point. E.g. if the branch.switch.before hook fails, the current branch is not switched and the after hook is never called. This is probably reasonable, but we need to be careful when the hooks are called. gel branch switch newbranch -c will call the "before" hook after the new branch is created, but before it is switched. So the command executes partially.

@aljazerzen
Copy link
Contributor Author

... will call the "before" hook after the new branch is created, but before it is switched

Oh. That's not good - it should be called before the branch is created. Execution of a command should be all or nothing.


Wait, are you saying that migration.apply.after should trigger after gel branch switch?

@aljazerzen aljazerzen merged commit 78a4441 into master Jan 31, 2025
17 checks passed
@aljazerzen aljazerzen deleted the hooks branch January 31, 2025 15:19
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