-
Notifications
You must be signed in to change notification settings - Fork 0
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 support for Git external sources #5
base: main
Are you sure you want to change the base?
Conversation
This commit adds Git external source support with `GitExternalSource`. The implementation is made extensible by adding an `ExternalSourceBase` as an abstract base class that creates external source objects based on the type parameter of the source descriptors. The git manipulation code takes a dependency on the `LibGit2Sharp` NuGet package.
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.
If it is not urgent, I will do a full review next pulse. I left some comments based on the things I noticed on a first look.
var fileContentLines = File.ReadAllLines(descriptorFilePath); | ||
var type = fileContentLines.FirstOrDefault(l => l.StartsWith("type"))?.Split("=").Last(); | ||
|
||
if (type is null) | ||
{ | ||
throw new ApplicationException($"{descriptorFilePath} is missing a type."); | ||
} | ||
|
||
switch (type) | ||
{ | ||
case "git": | ||
var repositoryUrl = fileContentLines.FirstOrDefault(l => l.StartsWith("repo"))?.Split('=').Last(); | ||
var branch = fileContentLines.FirstOrDefault(l => l.StartsWith("branch"))?.Split('=').Last(); | ||
var tag = fileContentLines.FirstOrDefault(l => l.StartsWith("tag"))?.Split('=').Last(); | ||
var commit = fileContentLines.FirstOrDefault(l => l.StartsWith("commit"))?.Split('=').Last(); |
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.
question: Why do you use a custom file format instead of established formats like json, yaml or even xml?
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.
It's an INI-style file. I aimed at a less verbose, to-the-point, and easily parse-able format. JSON is an option, although INI is even less verbose than that. But I don't think YAML or even XML are suitable, due to their unfriendliness.
|
||
// Reference precedence is commit, then tag, then branch. If all of them are null, the default | ||
// behavior is to check out the repo's default branch. | ||
return new GitExternalSource(new Uri(repositoryUrl), commit ?? (tag ?? branch)); |
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.
question: Why do you differentiate between commit, tag and branch? For git all three are a commit-ish that git can resolve. Especially, because there is no logic that would block someone from specifying a branch-name as a tag or commit.
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.
For readability's sake. At the end of the day, whatever you input will end up in the same git checkout
parameter, so there is no intrinsic distinction among the three. However, the file will make it obvious to the reader that you're specifying a branch, tag, or commit.
We can, however, change the property name to something like reference
and let the user input a branch, tag, or commit (but this needs to be made obvious in the file's documentation).
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.
I would favor using the git terminology commitish
. This is well established, but reference
also works and is commonly used.
src/DebianTarballBuilder.cs
Outdated
return false; | ||
switch (fileName) | ||
{ | ||
case "flamenco.external": |
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.
issue: This is a constant value that is defined here and in NamingConventions.ValidFlamencoFileNames
. This is an anti-pattern, because someone could edit just the value in one place and forget to edit the other.
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.
Done ✅
8129045
to
29a395f
Compare
This commit adds the external source functionality to the build debian-tarball command, as well as the file name expected. It is generally defined that a Flamenco descriptor file, such as the external source descriptor file, is a "flamenco file".
29a395f
to
2bf4d7b
Compare
This PR adds the external sources functionality to Flamenco as described in issue #3.
At first, this is an implementation for Git external sources. However, I tried to make this implementation extensible enough for us to be easily able to add more external source types, such as tarballs.
The external source is described in an INI-style configuration file defined as a "Flamenco file", which does not make it to the final
dist
result and is calledflamenco.external.{source_package}.{release}
. It describes the external source where to pull files from in the following format:The properties
commit
,tag
, andbranch
are mutually exclusive, wherecommit
takes precedence overtag
, which takes precedence overbranch
. If none of these are defined, the behavior is to checkout the repository's default branch.Closes #3.