-
Notifications
You must be signed in to change notification settings - Fork 0
Wrap scripts into a CLI tool #4
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
Conversation
400bc9d to
35d863c
Compare
35d863c to
8d804f3
Compare
DavidEdwards1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of small questions, but otherwise looks good!
| command: | ||
| description: "which command to run via the serverless-tools cli" | ||
| required: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IS the idea here that if (for whatever reason) we wanted to just deploy one function we could add it in the command input rather than having a second optional input (called function or something?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah exactly that. So in the action you just use it like this: https://github.com/fac/event-system/pull/36/files#diff-9b0a49d1840887e3bd80347f2a20c2323881c4bcb9e7ccbccb40ebcbb54675c9R36-R38
I think it's more (easily) extendable that was. Essentially the CLI is the action, so if you're adding to it you just add your code the CLI and you get the action functionality for free.
| desc "deploy", "publishes and deploys the specified lambda functions" | ||
| def deploy(action, function=nil) | ||
| Deployer.deploy(action: action, function: function) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be different from the comment command? Could they both be the same? e.g.
method_option :function, :type => :string, :aliases => "-f", :default => "{}"
def deploy(action, function=nil)
Deployer.deploy(action: action, function: options[:function])
end
or does that not work for some reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, they're different.
TBH, this was just a personal preference/judgement call. I felt the option was more appropriate for (comment) the command.
serverless comment -f '{"function": "Success"}'
vs
serverless deploy build
I guess my feeling was that the action for the deploy command (in this case build) was an extension of the command, where the function json was an argument. I'm totally happy to just have them the same, but that was the rationale.
DavidEdwards1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
The previous tools repo was set as individual scripts which which would be run via bundle. This changes the setup to be a gem which can be installed as an executable. It then exposes that executable via a Github action.
the lib folder is also more inline with a conventional ruby cli tool, where
ServerlessToolis introduced as a top level module.https://github.com/fac/data-platform/projects/4
Most of the code has not changed.
Once merged, I'll release a new version rerun: https://github.com/fac/event-system/pull/36