-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 Package Manager for GitHub Actions #11043
Changes from all commits
630b2c5
9a17f48
9368271
8a7ab19
140d2fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
# typed: strong | ||
# frozen_string_literal: true | ||
|
||
module Dependabot | ||
module GithubActions | ||
# Reference to the GitHub.com domain | ||
GITHUB_COM = T.let("github.com", String) | ||
|
||
# Regular expression to match a GitHub repository reference | ||
GITHUB_REPO_REFERENCE = T.let(%r{ | ||
^(?<owner>[\w.-]+)/ | ||
(?<repo>[\w.-]+) | ||
(?<path>/[^\@]+)? | ||
@(?<ref>.+) | ||
}x, Regexp) | ||
|
||
# Matches .yml or .yaml files in the .github/workflows directories | ||
WORKFLOW_YAML_REGEX = %r{\.github/workflows/.+\.ya?ml$} | ||
# Matches .yml or .yaml files anywhere | ||
ALL_YAML_FILES = %r{(?:^|/).+\.ya?ml$} | ||
|
||
# The ecosystem name for GitHub Actions | ||
ECOSYSTEM = T.let("github_actions", String) | ||
|
||
# The pattern to match manifest files | ||
MANIFEST_FILE_PATTERN = /\.ya?ml$/ | ||
# The name of the manifest file | ||
MANIFEST_FILE_YML = T.let("action.yml", String) | ||
# The name of the manifest file | ||
MANIFEST_FILE_YAML = T.let("action.yaml", String) | ||
# The pattern to match any .yml or .yaml file | ||
ANYTHING_YML = T.let("<anything>.yml", String) | ||
# The path to the workflow directory | ||
WORKFLOW_DIRECTORY = T.let(".github/workflows", String) | ||
# The path to the config .yml file | ||
CONFIG_YMLS = T.let("#{WORKFLOW_DIRECTORY}/#{ANYTHING_YML}".freeze, String) | ||
|
||
OWNER_KEY = T.let("owner", String) | ||
REPO_KEY = T.let("repo", String) | ||
REF_KEY = T.let("ref", String) | ||
USES_KEY = T.let("uses", String) | ||
STEPS_KEY = T.let("steps", String) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,18 +5,17 @@ | |
|
||
require "dependabot/file_fetchers" | ||
require "dependabot/file_fetchers/base" | ||
require "dependabot/github_actions/constants" | ||
|
||
module Dependabot | ||
module GithubActions | ||
class FileFetcher < Dependabot::FileFetchers::Base | ||
extend T::Sig | ||
extend T::Helpers | ||
|
||
FILENAME_PATTERN = /\.ya?ml$/ | ||
|
||
sig { override.params(filenames: T::Array[String]).returns(T::Boolean) } | ||
def self.required_files_in?(filenames) | ||
filenames.any? { |f| f.match?(FILENAME_PATTERN) } | ||
filenames.any? { |f| f.match?(MANIFEST_FILE_PATTERN) } | ||
end | ||
|
||
sig { override.returns(String) } | ||
|
@@ -49,9 +48,9 @@ def fetch_files | |
if incorrectly_encoded_workflow_files.none? | ||
expected_paths = | ||
if directory == "/" | ||
File.join(directory, "action.yml") + " or /.github/workflows/<anything>.yml" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we were looking only for |
||
File.join(directory, MANIFEST_FILE_YML) + " or /#{CONFIG_YMLS}" | ||
else | ||
File.join(directory, "<anything>.yml") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we are looking only for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is actually looking for files, more like providing examples... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's correct. |
||
File.join(directory, ANYTHING_YML) | ||
end | ||
|
||
raise( | ||
|
@@ -75,16 +74,19 @@ def workflow_files | |
# In the special case where the root directory is defined we also scan | ||
# the .github/workflows/ folder. | ||
if directory == "/" | ||
@workflow_files += [fetch_file_if_present("action.yml"), fetch_file_if_present("action.yaml")].compact | ||
@workflow_files += [ | ||
fetch_file_if_present(MANIFEST_FILE_YML), | ||
fetch_file_if_present(MANIFEST_FILE_YAML) | ||
].compact | ||
|
||
workflows_dir = ".github/workflows" | ||
workflows_dir = WORKFLOW_DIRECTORY | ||
else | ||
workflows_dir = "." | ||
end | ||
|
||
@workflow_files += | ||
repo_contents(dir: workflows_dir, raise_errors: false) | ||
.select { |f| f.type == "file" && f.name.match?(FILENAME_PATTERN) } | ||
.select { |f| f.type == "file" && f.name.match?(MANIFEST_FILE_PATTERN) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we were looking for |
||
.map { |f| fetch_file_from_host("#{workflows_dir}/#{f.name}") } | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,9 @@ | |
require "dependabot/errors" | ||
require "dependabot/file_parsers" | ||
require "dependabot/file_parsers/base" | ||
require "dependabot/github_actions/constants" | ||
require "dependabot/github_actions/version" | ||
require "dependabot/github_actions/package_manager" | ||
|
||
# For docs, see | ||
# https://help.github.com/en/articles/configuring-a-workflow#referencing-actions-in-your-workflow | ||
|
@@ -20,13 +22,6 @@ class FileParser < Dependabot::FileParsers::Base | |
|
||
require "dependabot/file_parsers/base/dependency_set" | ||
|
||
GITHUB_REPO_REFERENCE = %r{ | ||
^(?<owner>[\w.-]+)/ | ||
(?<repo>[\w.-]+) | ||
(?<path>/[^\@]+)? | ||
@(?<ref>.+) | ||
}x | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just moved into PackageManager as a constant There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TBH, this one feels even more generic, like it could be a top-level constant in utils or something. Package manager is fine too, just highlighting this is super generic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I generally try to add constants into package managers that will provide general information. But I can create another module that used for constants in ecosystem such as |
||
sig { override.returns(T::Array[Dependabot::Dependency]) } | ||
def parse | ||
dependency_set = DependencySet.new | ||
|
@@ -44,8 +39,24 @@ def parse | |
dependency_set.dependencies | ||
end | ||
|
||
sig { returns(Ecosystem) } | ||
def ecosystem | ||
@ecosystem ||= T.let( | ||
Ecosystem.new( | ||
name: ECOSYSTEM, | ||
package_manager: package_manager | ||
), | ||
T.nilable(Ecosystem) | ||
) | ||
end | ||
|
||
private | ||
|
||
sig { returns(Ecosystem::VersionManager) } | ||
def package_manager | ||
@package_manager ||= T.let(PackageManager.new, T.nilable(Dependabot::GithubActions::PackageManager)) | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Review Tip: The GitHub Actions version is extracted from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't make sense to me. GitHub Actions doesn't have a package manager. This is extracting dependency information and setting it as the PackageManager version? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh good catch Jake. I was assuming this PR was creating a generic package manager and then extracting the dependency and setting the name/version on the dependency. Creating a pseudo package manager for GitHub Actions so that we could code generic functionality across all package managers in core makes sense to me. But I agree with Jake that the version you're extracting shouldn't be tied to the package manager, but rather the dependency, which is updated by the package manager. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see your point. In this case it may be better to make package manager version not required since we don't have package manager and add another field to share this information that will show in metrics. |
||
|
||
sig { params(file: Dependabot::DependencyFile).returns(Dependabot::FileParsers::Base::DependencySet) } | ||
def workfile_file_dependencies(file) | ||
dependency_set = DependencySet.new | ||
|
@@ -94,20 +105,20 @@ def workfile_file_dependencies(file) | |
|
||
sig { params(file: Dependabot::DependencyFile, string: String).returns(Dependabot::Dependency) } | ||
def build_github_dependency(file, string) | ||
unless source&.hostname == "github.com" | ||
unless source&.hostname == GITHUB_COM | ||
dep = github_dependency(file, string, T.must(source).hostname) | ||
git_checker = Dependabot::GitCommitChecker.new(dependency: dep, credentials: credentials) | ||
return dep if git_checker.git_repo_reachable? | ||
end | ||
|
||
github_dependency(file, string, "github.com") | ||
github_dependency(file, string, GITHUB_COM) | ||
end | ||
|
||
sig { params(file: Dependabot::DependencyFile, string: String, hostname: String).returns(Dependabot::Dependency) } | ||
def github_dependency(file, string, hostname) | ||
details = T.must(string.match(GITHUB_REPO_REFERENCE)).named_captures | ||
name = "#{details.fetch('owner')}/#{details.fetch('repo')}" | ||
ref = details.fetch("ref") | ||
name = "#{details.fetch(OWNER_KEY)}/#{details.fetch(REPO_KEY)}" | ||
ref = details.fetch(REF_KEY) | ||
version = version_class.new(ref).to_s if version_class.correct?(ref) | ||
Dependency.new( | ||
name: name, | ||
|
@@ -124,7 +135,7 @@ def github_dependency(file, string, hostname) | |
file: file.name, | ||
metadata: { declaration_string: string } | ||
}], | ||
package_manager: "github_actions" | ||
package_manager: PackageManager::NAME | ||
) | ||
end | ||
|
||
|
@@ -139,11 +150,11 @@ def deep_fetch_uses(json_obj, found_uses = []) | |
|
||
sig { params(json_object: T::Hash[String, T.untyped], found_uses: T::Array[String]).returns(T::Array[String]) } | ||
def deep_fetch_uses_from_hash(json_object, found_uses) | ||
if json_object.key?("uses") | ||
found_uses << json_object["uses"] | ||
elsif json_object.key?("steps") | ||
if json_object.key?(USES_KEY) | ||
found_uses << json_object[USES_KEY] | ||
elsif json_object.key?(STEPS_KEY) | ||
# Bypass other fields as uses are under steps if they exist | ||
deep_fetch_uses(json_object["steps"], found_uses) | ||
deep_fetch_uses(json_object[STEPS_KEY], found_uses) | ||
else | ||
json_object.values.flat_map { |obj| deep_fetch_uses(obj, found_uses) } | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
# typed: strong | ||
# frozen_string_literal: true | ||
|
||
require "sorbet-runtime" | ||
require "dependabot/github_actions/constants" | ||
require "dependabot/github_actions/version" | ||
require "dependabot/ecosystem" | ||
require "dependabot/github_actions/requirement" | ||
|
||
module Dependabot | ||
module GithubActions | ||
class PackageManager < Dependabot::Ecosystem::VersionManager | ||
extend T::Sig | ||
|
||
# The package manager name for GitHub Actions | ||
NAME = T.let("github_actions", String) | ||
|
||
# The version of the package manager | ||
VERSION = T.let("1.0.0", String) | ||
|
||
sig { void } | ||
def initialize | ||
super( | ||
name: NAME, | ||
version: Version.new(VERSION) | ||
) | ||
end | ||
|
||
sig { override.returns(T::Boolean) } | ||
def deprecated? | ||
false | ||
end | ||
|
||
sig { override.returns(T::Boolean) } | ||
def unsupported? | ||
false | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
# typed: false | ||
# frozen_string_literal: true | ||
|
||
require "dependabot/github_actions/package_manager" | ||
require "dependabot/ecosystem" | ||
require "spec_helper" | ||
|
||
RSpec.describe Dependabot::GithubActions::PackageManager do | ||
let(:package_manager) { described_class.new } | ||
|
||
describe "#version_to_s" do | ||
it "returns the package manager version empty" do | ||
expect(package_manager.version_to_s).to eq("1.0.0") | ||
end | ||
end | ||
|
||
describe "#version_to_raw_s" do | ||
it "returns the package manager raw version empty" do | ||
expect(package_manager.version_to_raw_s).to eq("1.0.0") | ||
end | ||
end | ||
|
||
describe "#deprecated?" do | ||
it "returns always false" do | ||
expect(package_manager.deprecated?).to be false | ||
end | ||
end | ||
|
||
describe "#unsupported?" do | ||
it "returns always false" do | ||
expect(package_manager.unsupported?).to be false | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just moved into constants. Here we are looking for all
yml
,yaml
files.