Implement Composer hosted repositories #12
Conversation
| switch (assetKind) { | ||
| case ZIPBALL: | ||
| return doPutContent(path, tempBlob, content, assetKind); | ||
| return doPutContent(path, tempBlob, payload, assetKind); | ||
| case PACKAGES: |
There was a problem hiding this comment.
Trivial: PACKAGES, LIST, PROVIDER could be under the same case statement.
There was a problem hiding this comment.
Trivial: PACKAGES, LIST, PROVIDER could be under the same case statement.
Depends on how you feel about drop-through case statements. In my opinion they're a bad idea, so I generally tend to avoid them, and I'm not sure our internal coding standards have a preference or guideline one way or the other.
| @@ -185,13 +189,16 @@ protected Content doPutContent(final String path, | |||
|
|
|||
| Asset asset = getOrCreateAsset(path, assetKind, group, name, version); | |||
There was a problem hiding this comment.
Although not a change in this PR. This method also creates a component. Noting that here because someone could incorrectly use the getOrCreateAsset that does not create a component
There was a problem hiding this comment.
Although not a change in this PR. This method also creates a component. Noting that here because someone could incorrectly use the getOrCreateAsset that does not create a component
That's a good point, I'll break it out as a separate commit to rename those once I get this merged in.
| public class ComposerHostedDownloadHandler | ||
| implements Handler | ||
| { | ||
| @Nonnull |
There was a problem hiding this comment.
Trivial: I believe we now assume Nonnull so we only tend to annotate @'Nullable's
There was a problem hiding this comment.
Trivial: I believe we now assume Nonnull so we only tend to annotate @'Nullable's
I used to have issues with IDEA suggesting that it needed the annotation as the interface method still had it there, so I've tried to add @Nonnull as a result of that. If we're getting rid of @Nonnull then we should probably remove that from our codebase on the OSS side, then clean up the formats, including this one.
| switch (assetKind) { | ||
| case PACKAGES: | ||
| return HttpResponses.ok(hostedFacet.getPackagesJson()); | ||
| case LIST: |
There was a problem hiding this comment.
Trivial: No need to have this here other then to be explicit
There was a problem hiding this comment.
Trivial: No need to have this here other then to be explicit
That was my reasoning. :)
| private static final String ZIP_TYPE = "zip"; | ||
|
|
||
| private static final ObjectMapper mapper = new ObjectMapper(); | ||
|
|
||
| private static final DateTimeFormatter timeFormatter = DateTimeFormat.forPattern("yyyy-MM-dd'T'HH:mm:ssZZ"); |
There was a problem hiding this comment.
Add space below here
Done, 8142d1c.
| /** | ||
| * Builds a packages.json file as a {@code Content} instance containing the actual JSON for the given providers. | ||
| */ | ||
| private Content buildPackagesJson(final Repository repository, final Collection<String> names) throws IOException { | ||
| Map<String, Object> packagesJson = new LinkedHashMap<>(); | ||
| packagesJson.put(PROVIDERS_URL_KEY, repository.getUrl() + "/p/%package%.json"); |
There was a problem hiding this comment.
Trivial: Maybe make "/p/%package%.json" a constant
There was a problem hiding this comment.
Trivial: Maybe make "/p/%package%.json" a constant
Not a bad idea. 6ea0835
| Provider<SingleAssetComponentMaintenance> componentMaintenanceFacet | ||
|
|
||
| @Inject | ||
| Provider<HttpClientFacet> httpClientFacet |
There was a problem hiding this comment.
Question: Is this used anywhere?
It's used in the proxy recipe and was added to the hosted recipe, but we don't really need it in the hosted recipe for now. I lifted it up to the recipe support class because it was used in both places, but instead I'm just taking it out and pushing it back down to the proxy recipe for now:
Fixes #7 by adding an initial minimum-viable-product implementation of a Composer hosted repository. Source zips can be uploaded to an endpoint for that particular vendor, project, and version and the associated zip will be stored in the hosted repository with those coordinates. The documentation has also been updated with basic usage of the hosted repository feature.
Future improvements could include a less lazy approach to generating the associated packages and provider JSON since we're currently building this for each request based on the current database contents. We do this for other formats without a problem but in general isn't something that I'd advise in terms of scalability, but on the other hand, this also brings up some more difficult technical issues that would likely be better as a separate story. I'd rather see this out there as a way to solicit feedback than tackle that issue at the moment, particularly for an open-source repository format like this.
We should also go back and refactor some of this in separate PRs given the opportunity for further improvement, specifically 1) defining some kind of coordinate for Composer files rather than just passing strings/paths around (as that kind of manipulation is more error-prone) and 2) removing some of the static utility methods we have here as part of that refactoring in order to make unit-testing a bit easier. Neither of these, in my opinion, are necessary for the minimum viable product for Composer hosted.