Azure / container-scan

A GitHub action to help you scan your docker image for vulnerabilities
MIT License
219 stars 42 forks source link

Specify Trivy version in workflow file (version 2) #130

Closed oyri closed 2 years ago

oyri commented 2 years ago

To enable to use previous version av Trivy when breaking changes occur before container-scan action is updated. If not set, default is latest version.

This pull request replaces https://github.com/Azure/container-scan/pull/125 which has base release-branch, but the current has base master.

github-actions[bot] commented 2 years ago

This PR is idle because it has been open for 14 days with no activity.

koushdey commented 2 years ago

Hello @oyri, Thanks for raising the PR. I have a question in this change. There has been a recent breaking change in Trivy which we fixed in #123. Have you considered the case where the input version is an earlier version where we don't need to provide with image in the trivy command? Can you also handle that case in this PR?

koushdey commented 2 years ago

Since this is a replacement of #125. Request you to close it. Appreciate your contribution in making this action better 👍🏾

oyri commented 2 years ago

Hello @oyri, Thanks for raising the PR. I have a question in this change. There has been a recent breaking change in Trivy which we fixed in #123. Have you considered the case where the input version is an earlier version where we don't need to provide with image in the trivy command? Can you also handle that case in this PR?

I can do that, but my point with this PR was to support future breaking changes in Trivy, so the image/no image command is not that important, but for consistency it should be included.

I do not understand the build system her, master branches does not have the fix of #123? I will need to include this to handle the "image" command part, thus need to merge from the correct branch into mine.

koushdey commented 2 years ago

@oyri: This change has been included in #129 and #123 for master. I think you need to pull the latest changes from master. or create a new fork. Thanks for pointing it out. I also see the old changes in your files. For some reason the diff against the latest master is not reflecting properly in the PR

oyri commented 2 years ago

@oyri: This change has been included in #129 and #123 for master. I think you need to pull the latest changes from master. or create a new fork. Thanks for pointing it out. I also see the old changes in your files. For some reason the diff against the latest master is not reflecting properly in the PR

The first PR was against a release branch, but PR 123 was against master, and got included. Thus I created this PR from master instead. Why master does not contain any of the recent production changes might indicate a non-standard branch setup? Would be great if a maintainer of this repo could guide me through this?

koushdey commented 2 years ago

@oyri #128 is the PR for releases branch. I think both master and releases branch are in sync. I checked the commits on your forked branch. It is missing few commits from the master. Request you to update your branch with master. Also, can you run npm run-script build and also add the generated .js files in the PR?

FYI, for any contribution we accept the changes in master. And port the changes to the release branch. Code in releases actually run in actions.

oyri commented 2 years ago

Sorry about the back and forth here, now I have finally fetch the upstream, merged master and also run the build script.

koushdey commented 2 years ago

@oyri trivyHelper.test.ts tests are failing due to recent change. Either mock return latest for the trivyVersion or fix the failing case. You can run the tests using npm test. Suggestion for test changes

    const inputHelper = require('../src/inputHelper')
    inputHelper.trivyVersion = 'latest'
oyri commented 2 years ago

@oyri trivyHelper.test.ts tests are failing due to recent change. Either mock return latest for the trivyVersion or fix the failing case. You can run the tests using npm test. Suggestion for test changes

    const inputHelper = require('../src/inputHelper')
    inputHelper.trivyVersion = 'latest'

I added a new test as well, maybe not the best way to do it with the mocking, not familliar with Jest.