Skip to content

Conversation

@nozomuikuta
Copy link
Collaborator

@nozomuikuta nozomuikuta commented Sep 5, 2020

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

This PR implements #439, and lets us fetch repositories hosted on GitHub Enterprise, whose API has other domain than api.github.com.

I defined a Vuex getter githubUrls that returns an object whose values are full urls to get data of repository and its releases.

In addition, I modified related part of docs and SAO prompts.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes (if not applicable, please state why)

What and how I tested my changes

create-nuxt-content

First, I added a test case to check whether it creates files correctly when developer assign full url of repository instead of owner/repo string.

Second, I ran yarn test in the project root and all the tests were passed.

theme-docs

First, I ran yarn link for all the packages (@nuxt/content, @nuxt/theme-docs, and create-nuxt-content).

Second, I ran the linked create-nuxt-content to get theme-docs files.

Third, I replaced the installed @nuxt/theme-docs with my local one, and ran yarn run dev, resulting in that releases are correctly fetched.

Screen Shot 2020-09-06 at 0 15 54

It would be fine to test the full-url case with GitHub repository instead of GitHub Enterprise, because what is different is only the form of the original string, not actual reachability of the API.


P.S. This is my very first PR for open source project in my life. 👶

@nozomuikuta nozomuikuta changed the title Feat/dynamic GitHub url feat: support github custom domain Sep 5, 2020
@andoshin11
Copy link

LGTM

Copy link
Member

@atinux atinux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work done @nozomuikuta 👏

@nozomuikuta
Copy link
Collaborator Author

nozomuikuta commented Sep 8, 2020

@atinux

Thank you for reviewing!

In fact, I found some bugs in my PR. 🙇

That is, state.settings.github, which now can be either "owner/repo" or full url, is still used in 3 components (here, here, and here). I will modify these parts for them to handle getters.githubUrls object instead of state.settings.github, so that the correct url is rendered.

@nozomuikuta
Copy link
Collaborator Author

Sorry for multi-posting. I found a thing to reconsider.

According to GitHub Enterprise's spec, it is possible that host name for repositories and that for API are different. As far as I googled, there are some patterns like following:

Normal Root API Root To compute getters.githubUrls ...
http://github.com https://api.github.com Only "owner/repo" is necessary
http://my-company.com https://api.my-company.com 2 full urls to the repo and api are necessary
http://my-company.com https://my-company.com/api/v3 2 full urls to the repo and api are necessary

Fortunately, it seems that the path to edit files and that to get releases are always same.

  • Edit: path/to/repo/edit/branch/filename
  • Releases: path/to/repo/releases

@atinux

Then, I have a question about how to handle this issue.

Which do you think is approach that is most suitable for Nuxt's way or its DX?

Case 1

  • Add a prompt to create-nuxt-content for it to ask 2 urls, url for a repo and api-url for the repo.
    • In the case of GitHub, user can answer like owner/repo and skip the api-url.
    • In the case of GitHub Enterprise, user is promoted to answer 2 full urls.
    • In both cases, user can edit content/settings.json later.

Case 2

  • Revert the prompt that asks GitHub settings, so that it appears to ask a string in the form of "owner/repo".
    • In the case of GitHub Enterprise, user has to edit content/settings.json later.
    • In both cases, user can edit content/settings.json later.

Whichever, getters.githubUrls provides an object of urls, so all related UIs can render the urls correctly.

@atinux
Copy link
Member

atinux commented Sep 9, 2020

@nozomuikuta I prefer the Case 2, for advanced use case, they can edit settings.json, this way we keep as simple as simple the create-nuxt-content-docs :)

@nozomuikuta
Copy link
Collaborator Author

@atinux

I fixed my PR based on your opinion.

First, I reverted changes on create-nuxt-content-docs so it asks owner/repo string to cover a basic use case.

Second, I added githubApi setting for developer to define API full url besides github setting (cf. my comment above).

An alternative approach would be to make github setting able to be defined as a string, or an object which has 2 full urls. Since I though it simpler, I chose adding a setting over changing "type" of an existing one. But, I'm not sure which is more suitable so let me know if you have an opinion.

Third, I modified related parts of docs again (adding description about githubApi setting).

Forth, of course I ran yarn run test after development, and all the tests were passed :)

const { github = '', githubApi = '' } = state.settings

// GitHub Enterprise
if (github.startsWith('http') && githubApi.startsWith('http')) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it enough to check just existence of the 2 settings?

 if (github && githubApi) { ... }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be enough I guess

@nozomuikuta
Copy link
Collaborator Author

I noticed that this PR will have conflict with #468 in link computed property of AppGithubLink component. IMHO, it would be safer to merge #468 in advance so that I can rebase my branch and replace this.settings.github with this.githubUrls.repo.

@benjamincanac
Copy link
Member

@nozomuikuta Oh yes indeed, #468 has been merged!

@nozomuikuta nozomuikuta force-pushed the feat/dynamic-github-url branch from f7bd207 to 9033555 Compare September 14, 2020 17:51
@nozomuikuta
Copy link
Collaborator Author

@benjamincanac

Thank you for letting me know!
I rebased my branch, resolved conflicts, and pushed it again. 🙋‍♂️

@benjamincanac benjamincanac merged commit d01e134 into nuxt:dev Sep 15, 2020
nozomuikuta added a commit to nozomuikuta/nuxt-content that referenced this pull request Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support Custom Domain or API Root from which to fetch repository data

4 participants