Add GitLab as metadata-provider and make it easier to support other ones

[parallaxe]

This is the implementation based on my suggestion in https://forums.swift.org/t/pitch-package-collection-generator-supporting-multiple-git-hoster/53834/2.
Introducing a new field in the input.json to specify which metadata-provider should be used for a certain host.
Currently supported are github and gitlab. A default-mapping for the hosts github.com and gitlab.com is given.

Not sure if metadataProviderMapping for the input.json is descriptive enough, I'm open for suggestions.

[yim-lee]

Thank you @parallaxe for the PR. I think it's a great idea to add support for GitLab.

[yim-lee]

I think we should introduce PackageMetadataProviderType enum:

enum PackageMetadataProviderType {
    case github
    case gitlab
}

such that:

• PackageCollectionGeneratorInput.metadataProviderMapping would be of type [String: PackageMetadataProviderType] instead.
• There would be a dictionary metadataProviderByType: [PackageMetadataProviderType: PackageMetadataProvider] so GitHubPackageMetadataProvider/GitLabPackageMetadataProvider only needs to be initialized once.
• hostToProviderMapping: [String: PackageMetadataProvider] wouldn't be necessary I think, because we can do

let metadataProviderType = input.metadataProviderMapping[host]
let metadataProvider = metadataProviderByType[metadataProviderType]

Also, we should default metadataProvider to GitHubPackageMetadataProvider to keep the original behavior--i.e., the generator always tries fetching metadata through GitHub APIs even if user doesn't provide any tokens and/or configuration.

[parallaxe]

In my first try I implemented it this way, but afterwards I changed it to [String: String]. The main reason is the usability for the user. Currently, if the input.json contains a provider that is not implemented, the error-message will be "Provider \(provider) for host \(host) is not implemented". If the provider is an enum, the decoding of the JSON will fail with a message like "Cannot initialize PackageMetadataProviderType from invalid String value bitbucket". This wouldn't guide the user what is really going wrong. Implementing the init(from decoder: Decoder) throws-method and throw a more descriptive error, like this

enum PackageMetadataProviderType: String {
	case github
	case gitlab
}

extension PackageMetadataProviderType: Codable {
	enum Errors: Error, Equatable, CustomDebugStringConvertible {
		case providerNotImplemented(String)
		
		var debugDescription: String {
			switch self {
				case let .providerNotImplemented(provider):
				return "Provider '\(provider)' is not implemented"
			}
		}
	}
	
	init(from decoder: Decoder) throws {
		let container = try decoder.singleValueContainer()
		let provider = try container.decode(String.self)
		switch provider {
			case "github":
				self = .github
			case "gitlab":
				self = .gitlab
			default:
				throw Errors.providerNotImplemented(provider)
		}
	}
}

improves the situation, as the user will at least get a message like "Provider 'bitbucket' is not implemented". This would be acceptable, but from my point of view it is not as good as the current message. And it doesn't simplify the implementation. Would you still prefer this implementation, or do you see another possibility that I didn't think of?

Another point where I wasn't sure about: In my current implementation, there is one instance of a *MetaDataProvider-class per Host, each one with it's own instance of HTTPClient. I'm not sure if this may cause any problems, if one instance would handle multiple hosts (I don't see any state in it on a first glance that could cause problems, but I haven't studied it enough to rule it out). Like, if there are three different hosts that make use of the gitlab-API - may it cause a problem, if all of them use the same HTTPClient-instance?

Regarding making GitHubPackageMetadataProvider as the default: I prefer to not make an internal assumption of a default-provider. It would lead to confusing and inconsistent errors, if it tries to use the GitHub-API for other providers. The current error-message "Missing provider for host \(package.url.host ?? "invalid host"). Please specify it in the input-json in metadataProviders." does not only give an error what is missing, but also offers a guidance where to add it. As the providers are already setup for github.com, it would not change the current behaviour.

[yim-lee]

In my first try I implemented it this way, but afterwards I changed it to [String: String]. The main reason is the usability for the user.

I see. What if we keep metadataProviders [String: String] in PackageCollectionGeneratorInput but introduce an intermediate, internal method that converts it to [String: PackageMetadataProviderType]? The method can throw error with user-friendly message when it comes across a provider that it doesn't recognize.

Another point where I wasn't sure about: In my current implementation, there is one instance of a *MetaDataProvider-class per Host, each one with it's own instance of HTTPClient.

Yes, I don't think it's necessary to have one provider instance per host and that's why I suggest having metadataProviderByType: [PackageMetadataProviderType: PackageMetadataProvider] instead. I don't see a problem sharing the same provider instance across multiple hosts (HTTPClient is not tied to a host), but then I also don't think there would be so many different hosts that we would end up getting into trouble with one instance per host.

I do prefer singleton by provider type though.

I prefer to not make an internal assumption of a default-provider.

How about we add an option (e.g., --default-metadata-provider) that defaults to nil (none), but user can set it to one of PackageMetadataProviderType if they choose to?

[yim-lee]

Do license and README URLs serve raw contents of the files or they are rendered HTML pages?

For example, the README for this GitHub repo can be viewed at:

1. https://github.com/apple/swift-package-collection-generator/blob/main/README.md
2. https://raw.githubusercontent.com/apple/swift-package-collection-generator/main/README.md

We want URL no.2 in the package collection.

GitHub has separate APIs for fetching the "download URL" for license and README. I don't know if GitLab has the same? This is important for README especially.

[parallaxe]

It is the URL to the HTML rendered README. There is no raw-link that comes in the data provided from the GitLab-API, but I can build the path by my own (https://docs.gitlab.com/ee/api/repository_files.html#get-raw-file-from-repository). I will do that in another commit.

[yim-lee]

Perhaps refactor this into PackageMetadataProviderError so it can be shared across all providers.

[parallaxe]

I only realise now that there is also code inside SwiftPM to fetch metadata. And it seems like that the usage of the GitHubPackageMetadataProvider is hard-coded in it. This seems like that there should be a bigger consolidation overall. I agree with you that the error-codes should be shared, but I would either put them inside a new file inside this repository or defer it for a greater consolodation inside SwiftPM.
Is there anything planned to consolidate the metadata-provider-logic of SwiftPM and swift-package-collection-generator?

[yim-lee]

Is there anything planned to consolidate the metadata-provider-logic of SwiftPM and swift-package-collection-generator?

There is plan to move swift-package-collection-generator into SwiftPM actually. This repo started out separate from SwiftPM because we needed to be able to iterate on this more quickly, but now that things have stabilized, we would like to make this part of SwiftPM, which is more fitting. And when we do that, we will consolidate the code as you point out.

Let's just leave this code as-is for now.

[yim-lee]

Please add GitLabPackageMetadataProviderTests (see GitHubPackageMetadataProviderTests for example) that tests with real sample response payloads.

[parallaxe]

Yes, I will add them.

[yim-lee]

Please note that due to holidays there will be delays in response.

[parallaxe]

Thanks for the thoughtful review so far!

[yim-lee]

Just to be safe, we can guard instead of using !?

[quentinfasquel]

Any plans to support bitbucket as well ?

The PR is not complete yet thus should not be approved.

[domfz]

Is there any interest in completing this?

[parallaxe]

Interest yes, but sadly no planable timeslot from my side. If nothing higher priorized comes around the next weeks, I probably can work on it again this month. But this is basically the same if like in the last months, so...
But if anyone else is motivated and has time to take over, I would appreciate it.

[dervondenbergen]

Hey, just found this Repo / PR and was wondering, wether this issue is dead or if someone is still working on it?

[parallaxe]

Sorry from my side - as I'm no longer developing for iOS / macOS (neither privately nor for a company), I won't continue this PR. If anybody wants to take over, feel free.