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

Flags should have consitent api #1634

Closed

Conversation

theoboldalex
Copy link
Contributor

PR closes #1628 and is a follow on from the initial implementation of the --path flag merged in #1627

Provides a suite of CLI flags for local invocation of Bref event driven functions when not utilising Serverless framework. This makes the API for invoking event driven Bref functions consistent with running local functions using serverless bref:local

Available flags

Flag Description
-f The handler file
--path A path to a valid JSON file containing event data
--data A JSON string containing event data

Backward compatability

Backward compatibility with the current way to invoke bref-local
has been maintained so calling via vendor/bin/bref-local <handler> <event-data> will continue to work. Essentially, flags are 100% opt in.

Ordering of flags

The order of flags is now irrelevant and any combination of either -f && --path or -f && --data will be accepted as valid invocations.

Tested commands

I have tested invoking functions using bref-local in the following combinations.

Backward compatible commands

$ vendor/bin/bref-local index.php -> Hello World

$ vendor/bin/bref-local index.php '{"name": "Fred"}' -> Hello Fred

Flag based commands

$ vendor/bin/bref-local -f index.php -> Hello World

$ vendor/bin/bref-local -f index.php --path event.json -> Hello Fred

$ vendor/bin/bref-local -f index.php --data '{"name": "Alex"}' -> Hello Alex

$ vendor/bin/bref-local --path event.json -f index.php -> Hello Fred

$ vendor/bin/bref-local --data '{"name": "Alex"}' -f index.php -> Hello Alex

src/bref-local Outdated Show resolved Hide resolved
src/bref-local Show resolved Hide resolved
src/bref-local Outdated Show resolved Hide resolved
@theoboldalex
Copy link
Contributor Author

Thanks for the review @shadowhand

I have cleaned this up based on your suggestions.

Copy link
Contributor

@shadowhand shadowhand left a comment

Choose a reason for hiding this comment

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

Looks much cleaner, good stuff!

@theoboldalex
Copy link
Contributor Author

Any thoughts on this one @mnapoli ?

@mnapoli
Copy link
Member

mnapoli commented Oct 20, 2023

Hey, sorry for the delay. Big PRs are always harder to merge than simple ones 😬

There was a misunderstanding on my side, that's my fault. In the initial conversation, you mentioned 2 changes:

In the current implementation, when we pass the --path flag, it must be passed before the handler file.

When running Bref with Serverless, the handler file has the -f flag. I could add this to the non-Serverless bref-local script to ensure consistency in how functions are invoked locally regardless of whether we are using Serverless, CDK or Terraform.

I only considered the first suggestion, and I think solving this "bug" is great.

Regarding the second proposition, I'm not convinced. Yes, having consistency in flags could be good in theory, but:

  • those that use bref-local are usually not using Serverless Framework, so consistency with SF is not important
  • the SF API sucks IMO. I've been passing those -f function-name arguments since forever and I hate it

The Node world has a poor ecosystem for dealing with CLI flags and arguments. That's why they almost always use options (bref-local --option=x) instead of arguments (bref-local x). In the PHP world, the Symfony Console has set an invaluable precedent with very strong commands/sub-commands/args/options concepts, which always work well. I think we should take advantage of this for a better UX.

So here's why I'm really hesitant to merge this:

  • This PR changes the public API (even with backward compatibility), which is something we don't want to do without good reason
  • The SF API is worse, and aligning on this for consistency is not important and brings a worse UX for Bref users
  • This PR changes logic that's not heavily covered by tests, so I'm always even more hesitant to merge

All this is adding up. I'm sorry because of the time you invested in the PR, but I don't think it's worth merging in its current state 😬 Hope that makes sense.

I think the best option would be to only solve the 1st problem (--path should work if passed before or after arguments), if that's possible?

@theoboldalex
Copy link
Contributor Author

Thanks for the detailed feedback. @mnapoli

I'm not sure if it is possible to solve just the first issue using getopt but I can certainly take a look to see how feasible it is without any dependencies. I'll do some investigating and hopefully get some changes up for your consideration. Thanks 🙏

@mnapoli
Copy link
Member

mnapoli commented Mar 19, 2024

Closing inactive PRs, feel free to reopen.

@mnapoli mnapoli closed this Mar 19, 2024
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.

Bref-local should accept data from --path without Serverless framework
3 participants