Skip to content

Conversation

@sanket-mundra
Copy link
Contributor

Description

Please include a summary of the change, motivation and context.

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

@sanket-mundra sanket-mundra requested a review from a team June 20, 2023 13:19
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #241 (224bd05) into main (9342373) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main     #241   +/-   ##
=========================================
  Coverage     69.14%   69.14%           
  Complexity      650      650           
=========================================
  Files           110      110           
  Lines          4593     4593           
  Branches        493      493           
=========================================
  Hits           3176     3176           
  Misses         1157     1157           
  Partials        260      260           
Flag Coverage Δ
integration 69.14% <ø> (ø)
unit 52.80% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

github-actions bot commented Jun 20, 2023

Test Results

  59 files  ±0    59 suites  ±0   27s ⏱️ -14s
296 tests ±0  295 ✔️ ±0  1 💤 ±0  0 ±0 
298 runs  ±0  297 ✔️ ±0  1 💤 ±0  0 ±0 

Results for commit 224bd05. ± Comparison against base commit 9342373.

♻️ This comment has been updated with latest results.

@aaron-steinfeld
Copy link
Contributor

Also suggest moving the scan over either before this PR or as part of it, since right now no vuln scan is running to validate your changes. Example PR: hypertrace/java-grpc-utils#46

@sanket-mundra sanket-mundra marked this pull request as draft June 20, 2023 19:10
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@sanket-mundra
Copy link
Contributor Author

What should we do for vulnerability in snappy-java-1.1.8.4.jar?

@sanket-mundra
Copy link
Contributor Author

On local, dependencyCheckAggregate is passing with 0 vulns but here it's failing. Not sure what's going wrong.

@aaron-steinfeld
Copy link
Contributor

On local, dependencyCheckAggregate is passing with 0 vulns but here it's failing. Not sure what's going wrong.

Caching. If you do a dependencyCheckPurge for example first, it'll rebuild the DB to get the latest and you should see it.

@aaron-steinfeld
Copy link
Contributor

Fixed in hypertrace/java-grpc-utils#47

@sanket-mundra sanket-mundra marked this pull request as ready for review June 22, 2023 06:12
@sanket-mundra sanket-mundra requested review from a team as code owners June 22, 2023 06:12
github_token: ${{ secrets.GITHUB_TOKEN }}
files: ./**/build/test-results/**/*.xml

dependency-check:
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be part of pr-build.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Followed this to add depencycheck scan: hypertrace/java-grpc-utils#46 and here its part of pr-test.yml 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Two competing concerns here. The test job and build job have different triggers based on how they use secrets. We want to put all jobs under the pull_request trigger rather than pull_request_target unless there's a reason not to, so we'll need to take a look at how we can best arrange these so that we can get everything needed scheduled, but not over-expose on the the pull_request_target trigger. Will put something up later this morning.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK updated attribute service as an example - we don't really need that separation of pull_request and pull_request_target any more, so we can merge the two into a single workflow that gets scheduled with a few changes. You may want to do that in a separate PR though, up to you.

https://github.com/hypertrace/attribute-service/pull/170/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update once the above one gets merged.

pull_request_target:
branches:
- main

Copy link
Contributor

@ravisingal ravisingal Jun 22, 2023

Choose a reason for hiding this comment

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

add a cron also:

on:
  schedule:
    - cron: 10 0 * * 1

@ravisingal
Copy link
Contributor

ravisingal commented Jun 22, 2023

@sanket-mundra use latest github action for scanning image:

      - name: Run Trivy vulnerability scanner for kafka-zookeeper image
        uses: hypertrace/github-actions/trivy-image-scan@main
        with:
          image: hypertrace/entity-service
          output-mode: github

container:
image: bufbuild/buf:0.56.0
credentials:
username: ${{ secrets.DOCKERHUB_READ_USER }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't use creds here. This is using the old buf version, we should upgrade to the GA buf (1.0+).

That'll look like

      - name: Setup buf
        uses: bufbuild/buf-setup-action@v1
        with:
          github_token: ${{ github.token }}
      - name: Lint protos
        uses: bufbuild/buf-lint-action@v1
      - name: Check for breaking changes
        uses: bufbuild/buf-breaking-action@v1
        with:
          against: '.git#branch=origin/main'

It also means the buf config needs to be updated though so my suggestion for now would be to just remove the creds from here - let it run with anonymous pulls and we can come through and upgrade it after.

@sanket-mundra sanket-mundra merged commit 8216cf8 into main Jun 23, 2023
@sanket-mundra sanket-mundra deleted the fixVulnerabilities branch June 23, 2023 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants