Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(cli): Add a CLI to build deep-links to Central ELK #36
feat(cli): Add a CLI to build deep-links to Central ELK #36
Changes from 4 commits
8398944
54d8357
d4e9805
a60705e
9131b16
1ee363e
56a84ba
5cf85ac
30b2567
87f929b
824038d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Only compile for M series macs, because as a department, that's what we use for local development.
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 resulting binary is pretty big for what it does (>100MB). We could use bash instead of Deno, however we'd lose type-safety etc.
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.
It's because the Deno runtime has to be shipped within the binary. Can't be helped unfortunately (:
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.
If we're compiling to a specific arch anyway, is it worth using a compiled language like Rust or Go instead? Would likely cut down on the runtime size significantly.
Or compile to JS and run with the users existing node binaries?
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.
Chose TS on the assumption that it's the most familiar across the department (other than possibly Scala). If we move to JS, we could probably distribute this via NPM, rather than homebrew?
WDYT?
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.
I haven't worked with it much, but
deno install
could be a nice middle-ground, which wouldn't require much change to the code? https://docs.deno.com/runtime/manual/tools/script_installerThere 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.
@bryophyta my understanding is that
install
would create a script that invokes the CLI usingdeno
. That means that people would have to have deno installed in order to run the CLI, since the runtime is not included.I don't think we should require people to install additional software when running CLIs (which is why I'm personally averse to CLIs that need, for example, the JVM to run). I would rather have a 100MB standalone executable that's easy to distribute - I don't see it as different from a huge Electron app (which we've pretty much accepted as the norm).
As Ash says, there are languages there are better suited to writing CLIs, but Deno is a good compromise imho, since TypeScript is pretty much understood by everyone at the G.
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.
We distribute our dev tooling via homebew. I'd be keen for this to also be distributed the same way, to reduce cognitive load. Homebrew formulae supports the idea of dependencies, so we could craft this such that the raw TS files form the Homebrew artifact, and we have a dependency on Deno. Theoretically this would provide the same benefit as distributing a binary - the reduction of "works on my machine" symptoms.
I'd suggest we ship as is, and revise our approach once we understand what to optimise for (disk usage, performance, maintainability, etc.). Our work laptops have quite a lot of storage (1TB), and I'd be surprised if ~100MB is noticeable.
WDYT?
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.