Skip to content

Run nancy validation for all dependencies #1243

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 16, 2020
Merged

Run nancy validation for all dependencies #1243

merged 2 commits into from
Jul 16, 2020

Conversation

SVilgelm
Copy link
Member

No description provided.

@SVilgelm SVilgelm requested a review from a team July 14, 2020 15:35
@SVilgelm SVilgelm added area: tests Continuous integration, tests and other checks enhancement New feature or improvement labels Jul 14, 2020
sayboras
sayboras reviewed Jul 16, 2020
View reviewed changes
.github/workflows/pr.yml Outdated
@@ -15,6 +15,11 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: actions/setup-go@v2
Copy link
Member

@sayboras sayboras Jul 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

should it be in another job as this job is focusing on golangci-lint ? I think golangci-lint and nancy are two independent tasks, they can be running in parallel as well :)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, let me move it

sayboras
sayboras reviewed Jul 16, 2020
View reviewed changes
.github/workflows/pr.yml Outdated
# We cannot use nancy-github-action because it is outdated, so it's better to use the latest
# docker image for the validation
- name: nancy
run: go list -m all | docker run -i sonatypecommunity/nancy:latest
Copy link
Member

Choose a reason for hiding this comment

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

We might need to test it out for negative case to make sure it failed here. I faced a few cases github action didn't fail as expected, so just a precautious step.

Copy link
Member Author

@SVilgelm SVilgelm Jul 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm using it in several projects, and at work and it's failing :)

Copy link
Member Author

Choose a reason for hiding this comment

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

recently I had to disable it on a weekend because the server was unavailable

Copy link
Member

Choose a reason for hiding this comment

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

oh I see, we can just let it run as non-required job for sometime first, then change settings later once we see fit.

Copy link
Member Author

Choose a reason for hiding this comment

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

then I need to move it to a separate file

Copy link
Member Author

Choose a reason for hiding this comment

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

like extra

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

oh actually, you can still have it in the same pr workflow, mandatory setting is per job.

Anyway, no harm to keep it in another file.

@sayboras sayboras requested a review from a team July 16, 2020 01:16
@SVilgelm SVilgelm requested a review from sayboras July 16, 2020 01:33
@daixiang0
Copy link
Contributor

/lgtm

@SVilgelm
Copy link
Member Author

/lgtm

then approve? ;-)

iwankgb
iwankgb previously requested changes Jul 16, 2020
View reviewed changes
.github/workflows/pr-extra.yml Outdated
# We cannot use nancy-github-action because it is outdated, so it's better to use the latest
# docker image for the validation
- name: nancy
run: go list -m all | docker run -i sonatypecommunity/nancy:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

You should rather use:

go list -json -m all | nancy

(see deprecation notice)

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks I didn't know

Copy link
Member Author

Choose a reason for hiding this comment

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

done

sayboras
sayboras approved these changes Jul 16, 2020
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

LGTM apart for the point mentioned by @iwankgb

Update pr-extra.yml
d65e5b1 
Use `-json` flag
@SVilgelm SVilgelm merged commit 862ed88 into master Jul 16, 2020
@SVilgelm SVilgelm deleted the nancy branch July 16, 2020 22:14
@ldez ldez added this to the v1.29 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sayboras sayboras sayboras approved these changes

@iwankgb iwankgb iwankgb left review comments

Assignees
No one assigned
Labels
area: tests Continuous integration, tests and other checks enhancement New feature or improvement
Projects
None yet
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.
5 participants
@SVilgelm @daixiang0 @iwankgb @sayboras @ldez