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

feat(linter): expand package checks across multiple ecosystems #322

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hogo6002
Copy link
Contributor

This PR enhances the linter by adding package existence checks for a variety of ecosystems:

  • crates.io
  • npm
  • NuGet
  • RubyGems
  • Packagist
  • Pub
  • Hackage
  • Maven

To accommodate these additions, the ecosystems.go file has been refactored, with code related to package existence checks moved to package_check.go and version checks moved to version_check.go.

A current limitation is that malicious package record validation may fail, as these packages are often removed from package registries and are no longer queryable via API.

Signed-off-by: Holly Gong <gongh@google.com>
Signed-off-by: Holly Gong <gongh@google.com>
Signed-off-by: Holly Gong <gongh@google.com>
Signed-off-by: Holly Gong <gongh@google.com>
@hogo6002 hogo6002 force-pushed the add_ecosystem_check branch from 730f1e5 to b91e4e3 Compare December 19, 2024 02:36
Copy link
Collaborator

@andrewpollock andrewpollock left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

}

// EcosystemBaseURLs maps ecosystems to their base API URLs.
var EcosystemBaseURLs = map[string]string{
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's an opportunity to DRY this out a bit, and we can use the keys of EcosystemBaseURLs to infer what is now duplicated into SupportedEcosystems.

return true
}

// Needs to use GET instead of HEAD for Maven
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also add why to this comment

Suggested change
// Needs to use GET instead of HEAD for Maven
// Needs to use GET instead of HEAD for Maven.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a lot of (understandably valid) inconsistency to the implementation of the functions in this package. To assist with contributions, please add a package docstring that explains the goals of functions in this package and provides a canonical example that contributors can crib from, to avoid getting cargo-culted contributions from a non-canonical function implementation.

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