Add global debug option to flow command and correct flags #851
Conversation
Codecov ReportBase: 87.13% // Head: 87.14% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #851 +/- ##
==========================================
+ Coverage 87.13% 87.14% +0.01%
==========================================
Files 106 106
Lines 9091 9105 +14
==========================================
+ Hits 7921 7935 +14
Misses 689 689
Partials 481 481
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
feluelle
force-pushed
the
feature/debug
branch
from
5d88df6
to
caf75d6
Compare
feluelle
changed the title
feluelle
force-pushed
the
feature/debug
branch
2 times, most recently
from
1a1360e
to
94cff7b
Compare
feluelle
requested review from
andriisoldatenko,
aliotta,
neel-astro,
AmFlint,
kushalmalani,
sunkickr and
jemishp
as code owners
feluelle
force-pushed
the
feature/debug
branch
from
94cff7b
to
9978d76
Compare
|
@kushalmalani @andriisoldatenko Could you review this PR please? |
cmd/sql/flow_test.go
Outdated
| @@ -130,7 +130,7 @@ func TestFlowGenerateCmd(t *testing.T) { | |||
| err := execFlowCmd([]string{"init", projectDir}...) | |||
| assert.NoError(t, err) | |||
|
|
|||
| err = execFlowCmd([]string{"generate", "example_basic_transform", "--project-dir", projectDir}...) | |||
| err = execFlowCmd([]string{"generate", "example_basic_transform", "--project-dir", projectDir, "--verbose"}...) | |||
There was a problem hiding this comment.
Do we have tests for both when we enable --verbose and not? Do they have different assertions?
There was a problem hiding this comment.
Those tests are only testing that the inputs work with cobra. For example, if the flag is unknown, it will raise an error.
There was a problem hiding this comment.
We can add all flags at once to cover all scenarios of flags added to cobra.
| assert.NoError(t, err) | ||
|
|
||
| err = execFlowCmd([]string{"--debug", "run", "example_templating", "--env", "dev", "--project-dir", projectDir}...) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
Is checking if the command didn't raise an exception good enough? Would there be any other meaningful assertions we could make here?
There was a problem hiding this comment.
I think that this is currently enough as cobra will raise an error if the command is incorrect e.g. has too many args or unknown flags, etc.
feluelle
force-pushed
the
feature/debug
branch
from
8a02581
to
9701284
Compare
- add --verbose flag to validate and generate tests
feluelle
force-pushed
the
feature/debug
branch
from
9701284
to
db89ae0
Compare
| golang.org/x/net v0.0.0-20220624214902-1bab6f366d9e | ||
| golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a | ||
| golang.org/x/text v0.3.7 // indirect | ||
| golang.org/x/crypto v0.1.0 // indirect |
There was a problem hiding this comment.
Do we need changes in this file to be checked in?
There was a problem hiding this comment.
Yes, this can be useful to read. I am reading it right now.
There was a problem hiding this comment.
Why I had to run go mod tidy is because I added a new dependency golang.org/x/exp/slices.
Description
In the sql-cli we are adding a new global
--debugflag which we need to register to cobra, too. So that astro-cli users can use the flag in flow.🎟 Issue(s)
Related astronomer/astro-sdk#1040
📋 Checklist
make testbefore taking out of draftmake lintbefore taking out of draft