-
Notifications
You must be signed in to change notification settings - Fork 70
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
feature: Tracetest Linter 🐙 #2547
Conversation
…o feature/the-dream
…o feature/the-dream
@xoscar i have couple comments:
|
Moving @schoren slack comment to the PRs thread
|
looks good to me! |
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.
First of all: fantastic work, team! This feature is pretty cool!
I've added some comments about one little thing here or there in the code that we can improve, but that can be on future versions.
server/app/app.go
Outdated
@@ -23,6 +23,7 @@ import ( | |||
httpServer "github.com/kubeshop/tracetest/server/http" | |||
"github.com/kubeshop/tracetest/server/http/mappings" | |||
"github.com/kubeshop/tracetest/server/http/websocket" | |||
linter_resource "github.com/kubeshop/tracetest/server/linter/resource" |
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.
Nit: we should avoid snake_case
and use camelCase
here.
server/app/facade.go
Outdated
@@ -7,6 +7,7 @@ import ( | |||
"github.com/kubeshop/tracetest/server/executor" | |||
"github.com/kubeshop/tracetest/server/executor/pollingprofile" | |||
"github.com/kubeshop/tracetest/server/executor/trigger" | |||
linter_resource "github.com/kubeshop/tracetest/server/linter/resource" |
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.
Same here. Can we rename this to camelCase
?
server/executor/linter_runner.go
Outdated
|
||
"github.com/kubeshop/tracetest/server/analytics" | ||
"github.com/kubeshop/tracetest/server/linter" | ||
linter_resource "github.com/kubeshop/tracetest/server/linter/resource" |
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.
Can we change to camelCase
?
server/executor/linter_runner.go
Outdated
} | ||
} | ||
|
||
func (e *defaultlinterRunner) Runlinter(ctx context.Context, request LinterRequest) { |
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.
func (e *defaultlinterRunner) Runlinter(ctx context.Context, request LinterRequest) { | |
func (e *defaultlinterRunner) RunLinter(ctx context.Context, request LinterRequest) { |
dnsFields = []string{"net.peer.name", "http.url", "db.connection_string", "net.sock.peer.addr", "net.host.name"} | ||
) |
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.
To think in the future: can we open this configuration this somehow in the future (probably as an outside yaml)?
Some linters as ESLint (https://eslint.org/docs/latest/rules/array-callback-return) and Rubocop (https://docs.rubocop.org/rubocop/1.51/configuration.html) does this.
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.
Yes sir, we are going to have a bunch of fancy ways to configure the octolint
server/model/events/events.go
Outdated
TestID: testID, | ||
RunID: runID, | ||
Stage: model.StageTrace, | ||
Type: "TRACE_linter_START", |
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.
Type: "TRACE_linter_START", | |
Type: "TRACE_LINTER_START", |
server/model/events/events.go
Outdated
TestID: testID, | ||
RunID: runID, | ||
Stage: model.StageTrace, | ||
Type: "TRACE_linter_SKIPPED", |
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.
Type: "TRACE_linter_SKIPPED", | |
Type: "TRACE_LINTER_SKIPPED", |
server/model/events/events.go
Outdated
TestID: testID, | ||
RunID: runID, | ||
Stage: model.StageTrace, | ||
Type: "TRACE_linter_SUCCESS", |
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.
Type: "TRACE_linter_SUCCESS", | |
Type: "TRACE_LINTER_SUCCESS", |
server/model/events/events.go
Outdated
TestID: testID, | ||
RunID: runID, | ||
Stage: model.StageTrace, | ||
Type: "TRACE_linter_ERROR", |
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.
Type: "TRACE_linter_ERROR", | |
Type: "TRACE_LINTER_ERROR", |
@@ -62,7 +62,7 @@ describe('Transactions', () => { | |||
|
|||
cy.submitCreateForm('CreateTransactionFactory'); | |||
cy.get('[data-cy=transaction-details-name').should('have.text', `${name} (v1)`); | |||
cy.get('[data-cy=transaction-run-button]', {timeout: 30000}).should('be.visible'); | |||
cy.get('[data-cy=transaction-run-button]', {timeout: 50000}).should('be.visible'); |
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.
Should timeout
be a constant on this test file?
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 need to revamp the FE e2e tests, which would include cleaning up all of this 💩 more tests, better tests!
* feature: The Dream 💜 * feature(openapi): adding specs for linterns * updating types for lintern * updating types for lintern * adding lintern resource logic * feature(cli): fixing UI type * add runner structure * add rule to check if there are missing required attributes * add test to required attributes rule * registering lintern resource on app * registering lintern resource on app * feat(frontend): add linter settings page * adding lintern runner logic * registering lintern resource on app * feat(frontend): add LintResult component in test run page * first working version * fix(frontend): fix rule results * add not empty attribute rule * feat(frontend): add LinterResults component styles * add rule to validate attribute names * remove anonymous variables * rename files * add kind to span and add span naming rule * registering rules * finishing required attrs rule * feat(frontend): add dag lint icon and result click * feat(frontend): add linter loading component * adding common and security plugins * feat(frontend): add dag lint errors component * UI cleanup * UI cleanup * fix(frontend): fix styles and linter settings * feat(frontend): add octolint footer * fixing unit tests * fixing unit tests * fixing annoying typo * fixing e2e tests * cleanup changes * tracetest linter UX mvp improvements * tracetest linter UX mvp improvements * PR comments and beta badge --------- Co-authored-by: Matheus Nogueira <matheus.nogueira2008@gmail.com> Co-authored-by: Jorge Padilla <jorge.esteban.padilla@gmail.com>
This PR adds the first version of the Tracetest Linter to the project, including the following features:
Changes
Checklist
https://www.loom.com/share/577fd2cb4f3b45b6be9b8c6c6e0abfe0