Add apps open command #654
Conversation
cmd/meroxa/root/apps/open.go
Outdated
| err := open.Start(dashboardURL) | ||
| if err != nil { | ||
| o.logger.Errorf(ctx, "meroxa: can't open browser to URL %s: %s\n", dashboardURL, err) | ||
| //os.Exit(1) |
There was a problem hiding this comment.
Would this one be included or better be removed?
There was a problem hiding this comment.
oh, i meant to check that this was necessary from the code that i copied... it's hard to test because the binary being used is compiled in... oh well, i'll just remove it.
|
This already works great! One thing I noticed, that I'm not sure about if we should solve it:
If this is not a concern (e.g. |
cmd/meroxa/root/apps/open_test.go
Outdated
| desc: "Successfully open app link with arg", | ||
| appArg: "app-name", | ||
| path: func() string { | ||
| return filepath.Join("/tmp", uuid.New().String()) //nolint:gocritic |
There was a problem hiding this comment.
I wonder what linting error was ignored here. This brings up two more thoughts:
- Does
pathneed to be a function? seems like the function is defined exactly the same for each test case, we could remove thenolintif we droppath - Should we enable whyNoLint to have comments on
nolint? I guess this also depends on how manynolints are already used in CLI
There was a problem hiding this comment.
i want it to be a different path for each use case, so i don't want to reuse this path value, but not it doesn't need to be a function. i copied the nolint from somewhere else, but it seems like it doesn't need to be there at all!
|
|
||
| // open a browser window to the application details | ||
| dashboardURL := fmt.Sprintf("https://dashboard.meroxa.io/apps/%s/detail", nameOrUUID) | ||
| err := open.Start(dashboardURL) |
There was a problem hiding this comment.
It is a single error return. We can have this instead:
if err := open.Start(dashboardURL); err != nil {
o.logger.Errorf(ctx, "meroxa: can't open browser to URL %s: %s\n", dashboardURL, err)
}
There was a problem hiding this comment.
then i'd have to return nil on 102 because err wouldn't be defined. either way
There was a problem hiding this comment.
actually, i want to return the error and not just print it for proper exit codes. so then maybe i don't want to print it?
There was a problem hiding this comment.
$ m apps open --path ../demos/tenth
can't open browser to URL https://dashboard.meroxa.io/apps/tenth/detail
Error: forcing
looks like this now
@jayjayjpg yeah, I noticed that too. Everywhere that we mention the dashboard in the CLI, the URL is hardcoded to the prod version, so i think we're stuck with this for now, or we'd have to change something across the board (maybe checking one of the environment variables, which would be a small ticket) to have testing support for staging URLs. |
@janelletavares Sounds good and thank you for confirming! |
janelletavares
force-pushed
the
apps-open
branch
from
c9cedbb
to
698c314
Compare
Description of change
Convenience methods for seeing app details in the dashboard and interacting with it there.
Fixes https://github.com/meroxa/turbine-project/issues/219
Type of change
How was this tested?
Demo
Additional references
Documentation updated