Skip to content
This repository was archived by the owner on Aug 2, 2023. It is now read-only.

Conversation

@oyri
Copy link
Contributor

@oyri oyri commented Feb 3, 2022

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 #125 which has base release-branch, but the current has base master.

@github-actions
Copy link

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

@github-actions github-actions bot added the idle Inactive for 14 days label Feb 17, 2022
@koushdey
Copy link
Contributor

koushdey commented Mar 8, 2022

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 koushdey removed the idle Inactive for 14 days label Mar 8, 2022
@koushdey
Copy link
Contributor

koushdey commented Mar 8, 2022

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

@oyri
Copy link
Contributor Author

oyri commented Mar 8, 2022

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
Copy link
Contributor

koushdey commented Mar 8, 2022

@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
Copy link
Contributor Author

oyri commented Mar 9, 2022

@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
Copy link
Contributor

@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
Copy link
Contributor Author

oyri commented Mar 10, 2022

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

@oyri oyri requested a review from koushdey March 11, 2022 09:22
Copy link
Contributor

@koushdey koushdey left a comment

Choose a reason for hiding this comment

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

Missed a small suggestion earlier. Rest LGTM.

oyri and others added 2 commits March 11, 2022 13:58
Co-authored-by: Koushik Dey <57134947+koushdey@users.noreply.github.com>
@koushdey
Copy link
Contributor

koushdey commented Mar 14, 2022

@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
Copy link
Contributor Author

oyri commented Mar 14, 2022

@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.

Copy link
Contributor

@koushdey koushdey left a comment

Choose a reason for hiding this comment

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

Thanks for adding the new test. 👏🏾

@koushdey koushdey merged commit 2b9d165 into Azure:master Mar 14, 2022
koushdey added a commit that referenced this pull request Mar 23, 2022
* specify trivy version

* Trivy version logging

* Fix wrongly assigned variable for trivyversion.

* Updated readme with new parameter.

* Rename variable for Trivy version.

* Build JS

* Update src/trivyHelper.ts

Co-authored-by: Koushik Dey <57134947+koushdey@users.noreply.github.com>

* build trivyhelper.js - change debug message

* Fix failing tests and add new test

Co-authored-by: Koushik Dey <57134947+koushdey@users.noreply.github.com>
koushdey added a commit that referenced this pull request Mar 28, 2022
* specify trivy version

* Trivy version logging

* Fix wrongly assigned variable for trivyversion.

* Updated readme with new parameter.

* Rename variable for Trivy version.

* Build JS

* Update src/trivyHelper.ts

Co-authored-by: Koushik Dey <57134947+koushdey@users.noreply.github.com>

* build trivyhelper.js - change debug message

* Fix failing tests and add new test

Co-authored-by: Koushik Dey <57134947+koushdey@users.noreply.github.com>

Co-authored-by: Randi Øyri <randi.oyri@digdir.no>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants