Skip to content
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(CLI): Analyzer Resource Integration #2726

Merged
merged 6 commits into from Jun 15, 2023

Conversation

xoscar
Copy link
Collaborator

@xoscar xoscar commented Jun 13, 2023

This PR adds the actions and formatter to support the analyzer resource from the CLI

Changes

  • Adds analyzer actions
  • Adds analyzer formatter
  • Registers Analyzer actions to the resource actions object

Fixes

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

https://www.loom.com/share/45983054b6c548b6a21ed8b9a041936a

@xoscar xoscar added the CLI label Jun 13, 2023
@xoscar xoscar self-assigned this Jun 13, 2023
@xoscar xoscar linked an issue Jun 13, 2023 that may be closed by this pull request
@xoscar xoscar marked this pull request as ready for review June 13, 2023 21:41
@kdhamric kdhamric self-requested a review June 13, 2023 21:46
Copy link
Collaborator

@kdhamric kdhamric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Watched the loom - looked great!

Copy link
Contributor

@danielbdias danielbdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤘

Comment on lines +43 to +46
func (analyzer analyzerActions) Apply(ctx context.Context, fileContent file.File) (result *file.File, err error) {
result, err = analyzer.resourceClient.Update(ctx, fileContent, currentConfigID)
return result, err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (analyzer analyzerActions) Apply(ctx context.Context, fileContent file.File) (result *file.File, err error) {
result, err = analyzer.resourceClient.Update(ctx, fileContent, currentConfigID)
return result, err
}
func (analyzer analyzerActions) Apply(ctx context.Context, fileContent file.File) (*file.File, error) {
return analyzer.resourceClient.Update(ctx, fileContent, currentConfigID)
}

Comment on lines 18 to 19
// When I try to set up a config
// Then it should be applied with success
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// When I try to set up a config
// Then it should be applied with success
// When I try to set up an analyzer
// Then it should be applied with success

t.Run("get with no analyzer initialized", func(t *testing.T) {
// Given I am a Tracetest CLI user
// And I have my server recently created
// And no config previously registered
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// And no config previously registered
// And no analyzer previously registered

// And I have my server recently created
// And no config previously registered

// When I try to get a config on yaml mode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// When I try to get a config on yaml mode
// When I try to get an analyzer on yaml mode

Comment on lines 59 to 61
// And I have a config already set

// When I try to get a config on yaml mode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// And I have a config already set
// When I try to get a config on yaml mode
// And I have an analyzer already set
// When I try to get a analyzer on yaml mode

Comment on lines 39 to 41
// When I try to list config on pretty mode and there is no config previously registered
// Then it should print an empty table
// Then it should print a table with 4 lines printed: header, separator, the default config item and empty line
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// When I try to list config on pretty mode and there is no config previously registered
// Then it should print an empty table
// Then it should print a table with 4 lines printed: header, separator, the default config item and empty line
// When I try to list an analyzer on pretty mode and there is no analyzer previously registered
// Then it should print an empty table
// Then it should print a table with 4 lines printed: header, separator, the default analyzer item and empty line

Comment on lines 55 to 57
// And I already have a config created

// When I try to list a config by an invalid field
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// And I already have a config created
// When I try to list a config by an invalid field
// And I already have an analyzer created
// When I try to list one analyzer by an invalid field

Comment on lines 67 to 69
// And I already have a config created

// When I try to list config again on yaml mode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// And I already have a config created
// When I try to list config again on yaml mode
// And I already have an analyzer created
// When I try to list one analyzer again on yaml mode

Comment on lines 88 to 90
// And I already have a config created

// When I try to list config again on json mode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// And I already have a config created
// When I try to list config again on json mode
// And I already have an analyzer created
// When I try to list one analyzer again on json mode

Comment on lines 109 to 112
// And I already have a config created

// When I try to list config again on pretty mode
// Then it should print a table with 4 lines printed: header, separator, config item and empty line
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// And I already have a config created
// When I try to list config again on pretty mode
// Then it should print a table with 4 lines printed: header, separator, config item and empty line
// And I already have an analyzer created
// When I try to list one analyzer again on pretty mode
// Then it should print a table with 4 lines printed: header, separator, an analyzer item and empty line

Copy link
Contributor

@jorgeepc jorgeepc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a couple of questions.

@@ -0,0 +1,30 @@
package analyzer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the name of this file be delete_analyzer_test.go?

@@ -0,0 +1,108 @@
package analyzer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, get_analyzer_test.go?

@@ -0,0 +1,120 @@
package analyzer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list_analyzer_test.go?

@xoscar xoscar merged commit 707f918 into main Jun 15, 2023
24 checks passed
@xoscar xoscar deleted the 2722-analyzer-support-analyzer-resource-from-cli branch June 15, 2023 14:50
mathnogueira pushed a commit that referenced this pull request Jun 19, 2023
* feature(CLI): Analyzer Resource Integration

* feature(CLI): including e2e tests

* feature(CLI): including e2e tests

* feature(CLI): including e2e tests

* fixing comments

* fixing test file names
mathnogueira pushed a commit that referenced this pull request Jun 19, 2023
* feature(CLI): Analyzer Resource Integration

* feature(CLI): including e2e tests

* feature(CLI): including e2e tests

* feature(CLI): including e2e tests

* fixing comments

* fixing test file names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Analyzer] Support analyzer resource from CLI
4 participants