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 custom CLI commands to be added by extensions #7675

Merged
merged 5 commits into from Sep 7, 2021

Conversation

smilledge
Copy link
Contributor

✨️ Improvements

  • Added hooks to support custom CLI commands in extensions
  • Hooks will now be called when using the CLI (eg, item hooks, user hooks, etc)

🐛 Bugfixes

  • Fix typescript errors in the shared package (the api package has stricter tsconfg which caused errors in tests)

⚠️ Risks

  • Previously hooks weren't called from CLI commands (eg, creating a user now triggers user.create.before). However, I think this should be the expected behaviour?
    user hooks triggered
  • There may be additional console logs from loading extensions (depending on log level). This might be problematic if someone was depending on specific CLI output form a command.
    additional console logs

@rijkvanzanten
Copy link
Member

@nickrum Thoughts on this vs cli commands as an extension type? It's a strange mix 🤔 Theoretically, you could do custom endpoints through a custom hook as well by attaching to the app init hook. Maybe custom endpoints should just be another hook? Food for thought!

@nickrum
Copy link
Member

nickrum commented Sep 2, 2021

@rijkvanzanten I'd rather have it as a separate extension type. Right now, hooks are only meant for one-off actions at certain points within the lifecycle of Directus or to alter data-flow at certain user-triggered events. If we start to allow adding "additional functionality" through hooks, this breaks separation of concerns. It makes it much harder to reason, where new features are coming from.

Also, IMO the CLI integrated into the API is only meant for the bare minimum of commands necessary to make the Directus API usable. I'm not sure, if we should even support extending it at all.

@smilledge
Copy link
Contributor Author

smilledge commented Sep 2, 2021

IMO there's lots of valid reasons for extending the CLI. Some of my current use cases are;

  • Database seeding
  • Exporting & migrating collection metadata
  • Custom import & export scripts

Currently I'm just using a separate commander instance and importing the Directus API (eg, database, services, etc). This works, but it does have some disadvantages;

  • You don't get access to the Directus programmatic API
    • And it feels wrong to be importing modules with paths like directus/dist/utils/get-schema
  • It's messy and complicates the dev workflow 😅

Also, I think having the ability to add REST API endpoints using hooks is important too. Some situations where I'm currently using the routes.init hook over custom endpoints are;

  • Endpoints that need a specific path for compatibility with other systems and tools (eg, /healthz)
  • Replacing other systems with Directus while maintaining backwards compatibility

@smilledge
Copy link
Contributor Author

An alternative to adding CLI hooks could be to export the commander program in a way that it can be imported and extended. For example;

if (require.main === module) {
    createCli().parseAsync(process.argv).catch(...);
}

export function createCli(): Command {
    ...
}

@rijkvanzanten
Copy link
Member

If we start to allow adding "additional functionality" through hooks, this breaks separation of concerns. It makes it much harder to reason, where new features are coming from.

I agree, but I see that as 100% user-risk. We don't actively recommend nor demonstrate that way of injecting custom logic, but it's a very nice escape hatch to have for power users that know what they're doing (as demonstrated by @smilledge). I'm down with this addition 👍🏻

One point of concern you brought up earlier today @nickrum is the fact that this change requires the extensions to be initialized even though its only for the single CLI invocation. Is that a technical problem, or just something we need to take into account moving forwards?

Copy link
Member

@rijkvanzanten rijkvanzanten left a comment

Choose a reason for hiding this comment

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

LGTM

@rijkvanzanten rijkvanzanten added this to the v9.0.0-rc.93 milestone Sep 7, 2021
@rijkvanzanten rijkvanzanten merged commit 7186c4e into directus:main Sep 7, 2021
@nickrum
Copy link
Member

nickrum commented Sep 8, 2021

  • Endpoints that need a specific path for compatibility with other systems and tools (eg, /healthz)
  • Replacing other systems with Directus while maintaining backwards compatibility

With the recent changes to endpoints, you should be able to do this with a custom endpoint as well.

One point of concern you brought up earlier today @nickrum is the fact that this change requires the extensions to be initialized even though its only for the single CLI invocation. Is that a technical problem, or just something we need to take into account moving forwards?

Just something to take into account, especially concerning the upcoming changes that are necessary for OTA installation.

nickrum added a commit that referenced this pull request Sep 13, 2021
Fixes a regression introduced in #7675
nickrum added a commit that referenced this pull request Sep 13, 2021
Fixes a regression introduced in #7675
nickrum added a commit that referenced this pull request Sep 13, 2021
Fixes a regression introduced in #7675
rijkvanzanten pushed a commit that referenced this pull request Sep 13, 2021
Fixes a regression introduced in #7675
AustinPhillipTaylor pushed a commit to AustinPhillipTaylor/directus that referenced this pull request May 11, 2022
* Fix typescript errors in shared package

* Hooks for adding custom CLI commands

* Add CLI hooks to documentation
rijkvanzanten added a commit that referenced this pull request Oct 24, 2023
PR #7675 was intended to run hooks when using the users/roles command in the CLI. Using extensions everywhere can cause major issues, as the database may or may not exist, and may or may not be up to date.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants