Implement readTree for GitlabReader (#3547)

[mateiidavid]

Hey, I just made a Pull Request!

✔️ Checklist

• A changeset describing the change and affected packages. (more info)
• Added or updated documentation
• Tests for new functionality and regression tests for bug fixes
  - [ ] Screenshots attached (for UI changes)

What

---

Based on #3547 this PR adds an implementation for readTree in GitlabUrlReader. As part of this change, I have also added a new field to GitlabIntegrationConfig for apiUrl -- the field was present in both Bitbucket and Github configurations, so I thought it would make sense as part of this change to add it in Gitlab too.

The changes are quite straightforward, other than providing the implementation in the reader and touching the config, I have added in tests (with the same scenarios found in GH/Bitbucket readers) and made some smaller changes in the class, such as adding the TreeResponseFactory as a private field.

How

---

• The implementation is similar to the one in the Github reader (and Bitbucket reader branch suggested in Implement readTree on GitlabUrlReader #3547). The main distinctions are around the url for the archive and the archive format. From what I have noticed, Gitlab download urls (regardless of whether it is hosted) follow the same pattern: <protocol>://<host>/<org>/<repo>/-/archive/<branch-ref>/<repo>-<branch-ref>.<zip-fmt>.

• When it comes to the format of the archive I have chosen to go with .zip. I'm not sure what the general consensus around this is; the Bitbucket example used zip and GH unpacks tarballs. I have added in a TODO to keep track of this -- I can remove it or make the change directly based on feedback received.

• The final difference is in the path, Gitlab archives are named as repo-branch and contain the project tree, contrastingly, Github archives are named after a branch and have another directory inside with the repo name and the branch. Thought I'd put this in so it makes more sense when looking at the tests and the path.

Let me know of any feedback in terms of implementation details and code style. :)

I am also unsure if this should contain a changeset would appreciate any info related to it.

Signed-off-by: Matei David madavid@expediagroup.com

[changeset-bot]

🦋 Changeset detected

Latest commit: d8e9b75

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@backstage/backend-common	Patch
@backstage/integration	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[hooloovooo]

I think this looks like a good start! If you can get the tests fixed (mainly the call to getRequestOptions) i think we can merge this.

[mateiidavid]

I think this looks like a good start! If you can get the tests fixed (mainly the call to getRequestOptions) i think we can merge this.

Hi @hooloovooo, thank you for your comment and most of all apologies for the delay in pushing a fix! I have been away for a couple of days and did not get the chance to work on this.

I went ahead and fixed the call to getRequestOptions. I rebase off the latest upstream master, it seems the function got moved which made this much easier to reason with :). I also changed a test for the GitLab config class to add the default apiBaseUrl property.

Let me know what you think!

[benjdlambert]

Hey @mateiidavid this needs a changeset :)
https://github.com/backstage/backstage/blob/master/CONTRIBUTING.md#creating-changesets

[mateiidavid]

Hey @mateiidavid this needs a changeset :)
https://github.com/backstage/backstage/blob/master/CONTRIBUTING.md#creating-changesets

@benjdlambert ah thanks for bringing that up! First time contributing and I was unsure if a changeset is applicable here.

I went ahead and created a changeset for both backend-common and integration (seeing as I have made changes to the config). I gave both a minor version bump since the API since both the config and readTree method are backwards compatible.

Lmk if I should amend anything :) will be super prompt about it. Thanks for your time!

[benjdlambert]

Hey @mateiidavid perfect, one thing though. I think both of these changes can be patch changes, as we're below >1.0.0 then patch is essentially minor :)

[mateiidavid]

Hey @mateiidavid perfect, one thing though. I think both of these changes can be patch changes, as we're below >1.0.0 then patch is essentially minor :)

That makes a lot of sense, should have probably clocked on to it @benjdlambert 😅. I have manually changed it to be a patch now, let me know if there are any other changes or if I need to run yarn changeset again instead of manually changing the existing one.