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

Add checksum validation #166

Merged
merged 7 commits into from
Feb 19, 2024
Merged

Add checksum validation #166

merged 7 commits into from
Feb 19, 2024

Conversation

academo
Copy link
Member

@academo academo commented Feb 19, 2024

Add checksum validator with cli parameter

@academo academo requested review from a team as code owners February 19, 2024 09:17
@academo academo requested review from leventebalogh, wbrowne, andresmgot, oshirohugo and xnyo and removed request for a team February 19, 2024 09:17
@academo academo self-assigned this Feb 19, 2024
@academo academo added the enhancement New feature or request label Feb 19, 2024
SourceCodeDir string
ResultOf map[*Analyzer]interface{}
Report func(string, Diagnostic)
AnalyzerName string
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we are passing more global data to the analyzers I decided to move these parameters to its own object instead of having this function positional parameters growing ever more.

This required some minor changes in other test files and the runner but the functionality remains untouched

RootDir: filepath.Join("./"),
SourceCodeDir: filepath.Join("testdata", "version-match"),
RootDir: filepath.Join("./"),
CheckParams: &analysis.CheckParams{
Copy link
Member Author

Choose a reason for hiding this comment

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

Changing this since we moved to an object instead of positional parameters

if sourceCodeDir == "" {
// If no source code dir is provided, only report the result if ReportAll is set, for backwards compatibility
if sourceCodeNotProvided.ReportAll {
pass.ReportResult(pass.AnalyzerName, sourceCodeNotProvided, fmt.Sprintf("Source code not provided or the provided URL %s does not point to a valid source code repository", pass.SourceCodeDir), "If you are passing a Git ref or sub-directory in the URL make sure they are correct.")
pass.ReportResult(
Copy link
Member Author

Choose a reason for hiding this comment

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

breaking down these long lines for easier reading. no changes in text.

diags, err := runner.Check(passes.Analyzers, archiveDir, sourceCodeDir, cfg)
diags, err := runner.Check(
passes.Analyzers,
&analysis.CheckParams{
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the change from positional parameters to an object with global data we can pass to the analyzers.

Copy link
Member

@xnyo xnyo left a comment

Choose a reason for hiding this comment

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

Great work! 👏 👏

I have some comments, some of those like missing context.Context are general issues with the code of this repository, but I wanted to point them out anyways.

Apart from those, great work!! 🎉

pkg/analysis/passes/checksum/checksum.go Outdated Show resolved Hide resolved
pkg/analysis/passes/checksum/checksum.go Show resolved Hide resolved

resp, err := client.Get(url)
if err != nil {
logme.DebugFln("Error reading body: %s", err.Error())
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this is an error making the network request not while reading the body so I would change the logged message to something more clear

pkg/analysis/passes/checksum/checksum.go Show resolved Hide resolved
pkg/analysis/passes/checksum/checksum.go Outdated Show resolved Hide resolved
pkg/analysis/passes/checksum/checksum_test.go Show resolved Hide resolved
pkg/analysis/passes/checksum/checksum_test.go Show resolved Hide resolved
pkg/analysis/analysis.go Outdated Show resolved Hide resolved
pkg/analysis/passes/sourcecode/sourcecode.go Show resolved Hide resolved
pkg/cmd/plugincheck2/main.go Outdated Show resolved Hide resolved
@academo academo requested a review from xnyo February 19, 2024 14:04
Copy link
Member

@xnyo xnyo left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome 🎉

@academo academo merged commit dce3481 into main Feb 19, 2024
3 checks passed
@academo academo deleted the academo/validate-checksum branch February 19, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants