Initial commit #1

martinschaef · 2021-12-27T19:26:27Z

Description of changes:
Initial commit of the CLI.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

xxz-dev · 2021-12-30T00:56:15Z

README.md

+You can download the [aws-codeguru-cli](releases/download/latest/aws-codeguru-cli.zip) from the releases section.
+Download the latest version and add it to your `PATH`:
+```
+curl -OL https://github.com/martinschaef/aws-codeguru-cli/releases/download/latest/aws-codeguru-cli.zip


not sure if I'm doing something wrong, this downloaded file is incomplete somehow

➜ github-repo ll aws-codeguru-cli.zip -rw-r--r--@ 1 xxz staff 9 Dec 29 16:55 aws-codeguru-cli.zip ➜ github-repo unzip aws-codeguru-cli.zip Archive: aws-codeguru-cli.zip End-of-central-directory signature not found. Either this file is not a zipfile, or it constitutes one disk of a multi-part archive. In the latter case the central directory and zipfile comment will be found on the last disk(s) of this archive. unzip: cannot find zipfile directory in one of aws-codeguru-cli.zip or aws-codeguru-cli.zip.zip, and cannot find aws-codeguru-cli.zip.ZIP, period.

Ah snap ... I should have mentioned this ... you can't wget artifacts as long as the repo is private ... so this line can only be tested after we make it public :(

we should probably point it to https://github.com/aws/aws-codeguru-cli repository instead :)

yeah ... i'll fix that right after we go public ... then the link will work

xxz-dev · 2021-12-30T00:59:20Z

README.md

+```
+curl -OL https://github.com/martinschaef/aws-codeguru-cli/releases/download/latest/aws-codeguru-cli.zip
+unzip aws-codeguru-cli.zip
+export PATH=$PATH:./aws-codeguru-cli/bin


running aws-codeguru-cli prints following message. Is this expected?

➜ bin aws-codeguru-cli java.lang.NoSuchFieldException: mdc at java.base/java.lang.Class.getDeclaredField(Class.java:2411) at org.apache.log4j.MDCFriend.fixForJava9(MDCFriend.java:12) at org.slf4j.log4j12.Log4jMDCAdapter.<clinit>(Log4jMDCAdapter.java:37) at org.slf4j.log4j12.Log4j12ServiceProvider.initialize(Log4j12ServiceProvider.java:37) at org.slf4j.LoggerFactory.bind(LoggerFactory.java:152) at org.slf4j.LoggerFactory.performInitialization(LoggerFactory.java:139) at org.slf4j.LoggerFactory.getProvider(LoggerFactory.java:421) at org.slf4j.LoggerFactory.getILoggerFactory(LoggerFactory.java:407) at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:356) at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:382) at org.beryx.textio.AbstractTextTerminal.<clinit>(AbstractTextTerminal.java:27) at com.amazonaws.gurureviewercli.Main.main(Main.java:79) Usage: <main class> [options] Options: --build, -b Directory of all build artifacts. Can be used multiple times. --commit-range, -c Range of commits to analyze separated by ':'. For example HEAD^:HEAD --no-prompt Run in non-interactive mode. Default: false --output, -o Output directory. Default: ./code-guru --profile Use a named profile to get AWS Credentials --region Region where CodeGuru Reviewer will run. Default: us-east-1 * --repository, -r The directory of the git repo that should be analyzed. --src, -s Source directories to be analyzed. Can be used multiple times. Default: [./]

Oh wow ... I have not seen that before. Let me try to find out what that means

This is terrible ... this mix of log4j and slf4j lets me choose between an exception and a warning because something doesn't work with java9+ ... need to spend some more time on this

Should work in the last commit

xxz-dev · 2021-12-30T01:04:38Z

README.md

+```
+After compiling, we can run CodeGuru with:
+```
+aws-codeguru-cli -r ./ -b target/classes -s src -o ./output


it might be better to use the full name for the variable, it'll be more readable. aws-codeguru-cli --repository ./ --build target/classes --src src --output ./output

it'll be helpful to add the CLI usage description here.

Usage: <main class> [options] Options: --build, -b Directory of all build artifacts. Can be used multiple times. --commit-range, -c Range of commits to analyze separated by ':'. For example HEAD^:HEAD --no-prompt Run in non-interactive mode. Default: false --output, -o Output directory. Default: ./code-guru --profile Use a named profile to get AWS Credentials --region Region where CodeGuru Reviewer will run. Default: us-east-1 * --repository, -r The directory of the git repo that should be analyzed. --src, -s Source directories to be analyzed. Can be used multiple times. Default: [./]

xxz-dev · 2021-12-30T01:04:59Z

README.md

+CodeGuru produces a Json and Html report.
+
+
+### Running from CI/CD


can we also allow customer to pass in their existing s3 bucket?

CodeGuru will preform a full repository analysis if you do not provide a commit range. For pricing details see: https://aws.amazon.com/codeguru/pricing/ Do you want to perform a full repository analysis? (y/n): y CodeGuru Reviewer requires an S3 bucket to upload the analysis artifacts to. Do you want to create a new S3 bucket: codeguru-reviewer-cli-013991436161-us-east-1 codeguru-reviewer-cli-013991436161-us-east-1 (y/n): n Abort: CodeGuru needs an S3 bucket to continue.

Maybe ... personally, I cannot see myself using such a feature. Should we start minimal and add it if anyone asks?

we could!
My concern is that for most company users, the roles are usually strictly restricted. Only limited roles have permission to do CreatBucket action

Makes sense ... lemme add that in a bit

xxz-dev · 2021-12-30T01:07:01Z

README.md

-Be sure to:
+Before we start, let's make sure that you can access an AWS account from your computer. 
+Follow the credentials setup process for the for the [AWS CLI](https://github.com/aws/aws-cli#configuration).
+The credentials must have permissions to use CodeGuru Reviewer and S3.


we should explicitly call out what permissions are needed for this CLI. Might be a better experience to move the permission section up here.

xxz-dev · 2021-12-30T01:08:16Z

README.md

@@ -1,11 +1,102 @@
-## My Project
+# CodeGuru Reviewer CLI Wrapper
+Simple CLI wrapper for CodeGuru reviewer that provides a one-line command to scan a local clone of a repository and


we probably want to let customer know that this is not stand alone, and it does make API calls toward CodeGuru Reviewer SDK to get recommendations. It might also generate metering fees.

xxz-dev · 2021-12-30T01:17:49Z

README.md

+            "Effect": "Allow"
+        },
+        {
+            "Action": [


we'll also need s3:CreateBucket here

xxz-dev · 2021-12-30T01:18:48Z

README.md

+                "s3:CreateObject*",
+                "s3:GetObject*",
+                "s3:GetBucket*",
+                "s3:List*",
+                "s3:DeleteObject*",
+                "s3:PutObject",
+                "s3:Abort*"


are all of these permissions needed? can we only add the ones required?

Let me try that ... will have a smaller set in the next revision

xxz-dev · 2021-12-30T18:09:53Z

README.md

+        },
+        {
+            "Action": [
+                "s3:CreateBucket*",


s3:CreateBucket should be sufficient, we don't need * here. Generally, we want to be as specific as possible when requesting permissions.

will change

xxz-dev · 2021-12-30T18:12:11Z

README.md

+        {
+            "Action": [
+                "s3:CreateBucket*",
+                "s3:GetObject*",


is s3:GetObject sufficient? do we need * here?

xxz-dev · 2021-12-30T18:12:46Z

README.md

-* Change the title in this README
-* Edit your repository description on GitHub
+```json
+{


have you tried update the test role to have following permission policy and validate if it works?

meh ... good that you asked; scanning still worked, but creating new associations failed. I had to add 2 more S3 permissions.

xxz-dev · 2021-12-30T18:20:37Z

README.md

+```
+where `--repository .` specifies that the *repository* that we want to analyze is the current directory `./`. The option `--build target/classses` states that the build artifacts are located under `./target/classes` and `--src` says that we only want to analyze source files that are
+located under `./src`. The option `--output ./output` specifies where CodeGuru should write its recommendations to. By default,
+CodeGuru produces a Json and Html report.


let's also log the json file path in the output. :)

html report

html file is currently using rule id for rule field, consider adding a new field rule name and rule id to differentiate these two.

for line numbers, if start line and end line are the same, we can display one number; when they are different, we should show a range, instead of a single line. Example finding from amazon-codeguru-reviewer-sample-app/src/resources/setup.yml, line range should be line 9 ~ line 16.

is it better to sort the recommendations by severity or category? similar to what check style does.

Other than that, the currently html content looks good to me, we should it get it reviewed by doc writer, PM, and UX as well.

CodeGuru will preform a full repository analysis if you do not provide a commit range. For pricing details see: https://aws.amazon.com/codeguru/pricing/ Do you want to perform a full repository analysis? (y/n): y Starting analysis of /Users/xxz/workspace/test-repo/github-repo/amazon-codeguru-reviewer-sample-app with association arn:aws:codeguru-reviewer:us-east-1:013991436161:association:38fe98b9-05da-4980-a90b-6cbb663fc801 and S3 bucket codeguru-reviewer-cli-013991436161-us-east-1 2021-12-30 10:07:03,013 WARN - JAXB is unavailable. Will fallback to SDK implementation which may be less performant.If you are using Java 9+, you will need to include javax.xml.bind:jaxb-api as a dependency. Started new CodeGuru Reviewer scan: https://console.aws.amazon.com/codeguru/reviewer?region=us-east-1#/codereviews/details/arn:aws:codeguru-reviewer:us-east-1:013991436161:association:38fe98b9-05da-4980-a90b-6cbb663fc801:code-review:RepositoryAnalysis-codeguru-reviewer-cli-51b316a8-7ac2-42b2-920c-36ed3d1ebb20 .........................................................................................................................................................:) Directory ./output already exists; previous results may be overriden. Report with 11 recommendations written to: file:///Users/xxz/workspace/test-repo/github-repo/amazon-codeguru-reviewer-sample-app/output/codeguru-report.html Analysis finished.

Will address the comments above. As for:

is it better to sort the recommendations by severity or category? similar to what check style does.
my opinion is that you want all recommendations for one file next to each other, but that is subjective. If this thing catches on, we will probably spend more time on the html report and add a little JS so you can sort and filter.

xxz-dev · 2022-01-06T18:27:35Z

.github/workflows/guru-reviewer.yml

+      continue-on-error: true
+      id: iam-role
+      with:
+        role-to-assume: arn:aws:iam::048169001733:role/GuruGitHubCICDRole


have we double checked with security team on if it's okay to expose the role ARN in public Github repository?

Security review is ongoing ... its a question in the ticket, but no answer yet

xxz-dev · 2022-01-06T18:31:29Z

settings.gradle

@@ -0,0 +1,3 @@
+// Always define a settings file:
+// https://docs.gradle.org/current/userguide/organizing_gradle_projects.html#always_define_a_settings_file
+rootProject.name = 'aws-codeguru-cli'


should we check with PM/Doc writer to validate if we wanted to call itamazon-codeguru-reviewer-cli?

yup. I'm on Amy's calendar

xxz-dev · 2022-01-06T18:32:23Z

src/main/java/com/amazonaws/gurureviewercli/Main.java

+import com.amazonaws.services.securitytoken.model.GetCallerIdentityRequest;
+
+public class Main {
+    private static final String FES_ENDPOINT_PATTERN_PROD = "https://codeguru-reviewer.%s.amazonaws.com";    


nit: given this will be public, let's rename it to REVIEWER_ENDPOINT_PATTERN?

xxz-dev · 2022-01-06T18:34:35Z

src/main/java/com/amazonaws/gurureviewercli/Main.java

+    private String profileName;
+
+    @Parameter(names = {"--commit-range", "-c"},
+               description = "Range of commits to analyze separated by ':'. For example HEAD^:HEAD ",


nit: does it also support commit id as input to define the range? Might be useful to add an example?

using a full hash is rather long, and at least the HEAD^:HEAD is something that you copy and use. I think people will understand that SHAs work if shorthands work

xxz-dev · 2022-01-06T18:36:36Z

src/main/java/com/amazonaws/gurureviewercli/Main.java

+    private String outputDir = "./code-guru";
+
+    @Parameter(names = {"--bucket-name"},
+               description = "Optional S3 Bucket name. Bucket name has to start with 'codeguru-reviewer-'")


We might want to add a bit more explanation about what this S3 bucket will be used for

nit: will be helpful to make it consistent to call out whether a field is Required or Optional

Yeah, I'll clarify

xxz-dev · 2022-01-06T19:16:08Z

src/main/java/com/amazonaws/gurureviewercli/adapter/ScanAdapter.java

+                                                    final GitMetaData gitMetaData) {
+
+        val hasDiff = gitMetaData.getBeforeCommit() != null && gitMetaData.getAfterCommit() != null;
+        val eventInfo = hasDiff ? new EventInfo().withName("push") : new EventInfo().withName("schedule");


what's the difference between push and pull_request? can you check with CP team on which value is more appropriate?

I triggered a ton of Sev2s to figure this out. pull_request didn't work for me back then

xxz-dev · 2022-01-06T19:17:18Z

src/main/java/com/amazonaws/gurureviewercli/model/ErrorCodes.java

+    GIT_BRANCH_MISSING("Cannot determine Git branch"),
+    GIT_REMOTE_MISSING("Cannot identify Git remote URL"),
+    GIT_INVALID_COMMITS("Not a valid commit"),
+    GIT_EMPTY_DIFF("Diff is empty"),


"Git diff is empty"

src/main/java/com/amazonaws/gurureviewercli/util/Log.java

xxz-dev · 2022-01-06T19:20:05Z

src/main/java/com/amazonaws/gurureviewercli/util/ZipUtils.java

+                                zs.write(Files.readAllBytes(path));
+                                zs.closeEntry();
+                            } catch (Exception e) {
+                                log.error(e);


silent exception?

yeah ... i haven't see this happen and I want it to be reported if people see it. Also, i don't want the entire tool to fail just because one file cannot be added to the zip.

should we print out the message that Skipping file xxx due to yyy to communicate to the user?

xxz-dev · 2022-01-06T19:20:15Z

src/main/java/com/amazonaws/gurureviewercli/util/ZipUtils.java

+                                zs.closeEntry();
+                            } catch (Exception e) {
+                                log.error(e);
+                            }


silent exception?

yeah ... i haven't see this happen and I want it to be reported if people see it. Also, i don't want the entire tool to fail just because one file cannot be added to the zip.

same as above, it's okay to suppress it, we should communicate the skip logic to the user though

xxz-dev

I tested the happy case and verified it works. For sad case / edge cases, can you help validate the tool is performing as expected?

xxz-dev

Changes LGTM. Please get security and doc writer approval prior to merging this change.

…ersion

xxz-dev

have we tried run CGR on this package and see if there any findings we should address? :p

xxz-dev · 2022-01-21T01:04:44Z

src/main/java/com/amazonaws/gurureviewercli/Main.java

+            } finally {
+                if (scanMetaData != null) {
+                    // try to clean up objects from S3.
+                    main.tryDeleteS3Object(config.getS3Client(),


does the credential have permission to delete s3 object?

It does. Let me add it to the readme

xxz-dev · 2022-01-21T01:05:22Z

src/main/java/com/amazonaws/gurureviewercli/Main.java

+            if (s3Key != null) {
+                s3Client.deleteObject(DeleteObjectRequest.builder().bucket(s3Bucket).key(s3Key).build());
+            }
+        } catch (Throwable e) {


instead of catching Throwable, can we catch Exception instead?

xxz-dev

LGTM!

sync

Martin Schaef added 19 commits December 17, 2021 13:33

initial commit

29b0da1

fixed build logic

b65b0aa

adding actions

9959fc4

action fixes

d0d24a6

more logging

3f0e2cb

switch region for self test

a87c09d

adding deployment action

69f460f

release stuff

b248dfb

trying

afda619

clean up dependencies

95261b6

build magic

e414cef

work work

a18e516

work work

d943a34

work work

6c54b31

working now

dc82d8e

working now

bc29f51

working now

7242223

looks complete

fbe710c

working on the readme

87e37af

martinschaef requested a review from xxz-dev December 27, 2021 19:26

xxz-dev reviewed Dec 30, 2021

View reviewed changes

improved readme, clean up dependencies, build with java matrix

e9c4d39

martinschaef requested a review from xxz-dev December 30, 2021 16:32

xxz-dev reviewed Dec 30, 2021

View reviewed changes

improvements based on xxz's comments

26d33e7

martinschaef requested a review from xxz-dev December 30, 2021 20:10

do not fail if git remote is unknown

94735b7

xxz-dev reviewed Jan 6, 2022

View reviewed changes

addressing review comments

9732a40

martinschaef requested a review from xxz-dev January 6, 2022 21:08

Martin Schaef added 3 commits January 7, 2022 06:38

addressing next round of comments

7416eef

updating gitignore

5e1305a

fix debug message

9c43535

xxz-dev approved these changes Jan 10, 2022

View reviewed changes

Martin Schaef added 8 commits January 11, 2022 09:01

bug fixes and making the git folder not required for package scans

030823e

changing long param name from repository to root-dir

c71b9f4

typo

4966c48

bugfix

c974590

remove unnecessary field

9b31d0a

typo

6976f2d

upgrading to aws sdk v2 and some bug fixes from the builder toolbox v…

de2f054

…ersion

Better error handling if association fails. Support for KMS keys

92500e1

xxz-dev reviewed Jan 21, 2022

View reviewed changes

xxz-dev approved these changes Jan 21, 2022

View reviewed changes

Addressing xxz's comments

2d6d824

martinschaef merged commit 1daafe6 into aws:main Jan 27, 2022

awsgaucho pushed a commit that referenced this pull request Mar 28, 2022

Merge pull request #1 from aws/main

aca5e9c

sync

		CodeGuru produces a Json and Html report.


		### Running from CI/CD

Initial commit #1

Initial commit #1

Conversation

martinschaef commented Dec 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinschaef Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxz-dev left a comment

Choose a reason for hiding this comment

xxz-dev left a comment

Choose a reason for hiding this comment

xxz-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxz-dev left a comment

Choose a reason for hiding this comment

martinschaef Jan 6, 2022 •

edited

Loading