Skip to content
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

support Gitlab self hosted instance (#89) #95

Merged
merged 3 commits into from
Aug 2, 2018

Conversation

xuchaoying
Copy link
Contributor

@xuchaoying xuchaoying commented May 31, 2018

What: allow user to input private_token to authenticate with the GitLab API

Why: self hosted Gitlab instance not working due to the api failing #89

How: basically add a question to ask for user private toekn, and append the private token as another param to the gitlab apis.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@codecov-io
Copy link

codecov-io commented Jun 1, 2018

Codecov Report

Merging #95 into master will increase coverage by 0.34%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
+ Coverage   57.83%   58.18%   +0.34%     
==========================================
  Files          19       19              
  Lines         434      440       +6     
  Branches       73       77       +4     
==========================================
+ Hits          251      256       +5     
- Misses        152      153       +1     
  Partials       31       31
Impacted Files Coverage Δ
src/repo/index.js 84.61% <100%> (+0.4%) ⬆️
src/repo/gitlab.js 44.73% <85.71%> (+5.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 533813b...7224284. Read the comment docs.

Copy link
Collaborator

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Changes all look good (looks like it's been a while since prettier was run on these files, sorry about that). Please just rename private_token to privateToken.

Something to consider... Is there a security concern by including a private token committed in this file in the repository? I'm kinda thinking that we shouldn't allow that and instead should read an environment variable or something....

README.md Outdated
@@ -158,6 +158,7 @@ These are the keys you can specify:
--> `all-contributors-cli`. Mandatory.
* `repoType`: Type of repository. Must be either `github` or `gitlab`. Default: `github`.
* `repoHost`: Points to the repository hostname. Change it if you use a self hosted repository. Default: `https://github.com` if `repoType` is `github`, and `https://gitlab.com` if `repoType` is `gitlab`.
* `private_token`: The personal access token to to authenticate with the GitLab API. Offer it if you use a self hosted repository. Default: `''`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this follow the existing camel casing convention? It should be privateToken.

@xuchaoying
Copy link
Contributor Author

Yes, an environment variable is better considered to the security problem. I'll change it later

@hipstersmoothie
Copy link
Contributor

hipstersmoothie commented Jun 8, 2018

@kentcdodds @xuchaoying I am going to be opening a PR to make this repo work with Github Enterprise. The private token code is needed! I will reference this PR in mine.

@chris-dura
Copy link
Contributor

really looking forward to getting this merged in...

@kentcdodds
Copy link
Collaborator

Sorry, there are a bunch of merge conflicts here that need to be resolved :-/

@xuchaoying
Copy link
Contributor Author

sorry for the belated commit. :(

Private token needs to be set as an environment variable now. @kentcdodds

README.md Outdated
# set private token on linux
export PRIVATE_TOKEN=your_privete_token
# set private token on windows
set PRIVATE_TOKEN=your_privete_token
Copy link
Contributor

Choose a reason for hiding this comment

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

privete -> private

Copy link
Contributor

@chinesedfan chinesedfan left a comment

Choose a reason for hiding this comment

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

@xuchaoying Just share some comments for you. (I am not a real maintainer) :)

@@ -38,10 +43,14 @@
"yargs": "^10.0.3"
},
"devDependencies": {
"kcd-scripts": "^0.29.0",
"kcd-scripts": "^0.30.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

It solves #105. Good job!

README.md Outdated
@@ -137,6 +137,18 @@ Where `username` is the user's GitHub or Gitlab username, and `contribution` is
* tutorial: [✅](# "Tutorials")
* video: [📹](# "Videos")

Please note that if you are using a self-hosted gitlab instance, before add
Copy link
Contributor

Choose a reason for hiding this comment

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

before adding? Haha.

"files": ["dist"],
"files": [
"dist"
],
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know whether maintainers care these format changes or not. But without them, it is easy to review and track history logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these changes are generated by prettier automatically on commit (?), and prettier hadn't been run in awhile. So, these whitespace changes are probably the correct format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chris-dura yes, maybe it's prettier. I didn't change the format at all.

Copy link
Collaborator

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Super! Thank you :)

@kentcdodds kentcdodds merged commit 0d1346d into all-contributors:master Aug 2, 2018
Berkmann18 pushed a commit that referenced this pull request May 24, 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.

None yet

6 participants