Skip to content
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.

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

Test distro and mock dnf json cleanup #159

Merged
merged 7 commits into from
Sep 15, 2023

Conversation

achilleas-k
Copy link
Member

The dnfjson_mock package isn't used by the images project and was left over from osbuild-composer. The "test-distro-2" test distribution that was calling it was never used in the images tests. The mock-dnf-json cmd is also duplicated in osbuild-composer and is used there by API tests.

The PR also includes a very small functional change: It adds the package name and digest to the error message when the digest/checksum fails to validate.

CI changes: I updated the version of golangci-lint that we use in the GitHub actions and disabled gosec G101 (hardcoded credentials) because it produces too many (and exclusively) false positives. The updated linter revealed a lot of instances of G601 (implicit memory aliasing in for loop) when I ran it locally and I fixed all of them.

When a package's checksum is invalid, include the name and digest in the
error message for easier troubleshooting.
The dnfjson_mock package isn't used by the images project and was left
over from osbuild-composer.

The "test-distro-2" test distribution that was calling it was never used
in the images tests.

The mock-dnf-json cmd is also duplicated in osbuild-composer and is used
there by api tests.
Update the golangci-lint binary in the github action to use the latest
version.
The check is very sensitive and returns only false positives: content
hashes and test strings.
Fix all instances of gosec G601: Implicit memory aliasing in for loop.
thozza
thozza previously approved these changes Sep 13, 2023
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

Nice work!

bcl
bcl previously requested changes Sep 13, 2023
Copy link
Contributor

@bcl bcl left a comment

Choose a reason for hiding this comment

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

Tried updating osbuild-composer with the changes and ran into at least 1 problem.

pkg/distro/test_distro/distro.go Show resolved Hide resolved
@achilleas-k
Copy link
Member Author

Should we keep this PR open until osbuild/osbuild-composer#3683 gets worked out in case we need to bring some changes here?

@bcl bcl dismissed their stale review September 13, 2023 21:57

fixed things

@bcl
Copy link
Contributor

bcl commented Sep 13, 2023

This just needs one commit from this draft (I can never seem to make adding a commit to an existing PR work) - 5325bf5 to switch newTestDistro to a public function.

I've updated my PR osbuild/osbuild-composer#3683 to use this, and to fix the cloudapi test failures I was seeing. That PR will of course fail until a new images is vendored, I can do that once this PR is merged.

@achilleas-k
Copy link
Member Author

This just needs one commit from this draft (I can never seem to make adding a commit to an existing PR work) - 5325bf5 to switch newTestDistro to a public function.

Cherry picked and pushed. Thanks!

Copy link
Contributor

@bcl bcl left a comment

Choose a reason for hiding this comment

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

Works for me :)

@bcl bcl added this pull request to the merge queue Sep 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Sep 14, 2023
@achilleas-k achilleas-k added this pull request to the merge queue Sep 15, 2023
@achilleas-k
Copy link
Member Author

achilleas-k commented Sep 15, 2023

The GitLab CI test generator failed (looks like a flake). Adding back on the queue.

Merged via the queue into osbuild:main with commit e9ea335 Sep 15, 2023
7 checks passed
@achilleas-k achilleas-k deleted the test-cleanup branch December 14, 2023 10:49
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.

3 participants