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

Read access token from config #135

Merged
merged 8 commits into from Nov 6, 2019

Conversation

@xiaozhikang0916
Copy link
Contributor

xiaozhikang0916 commented Jul 9, 2019

I would like to deploy my blog page from some CI servers, but found that it's dangerous to store my account info in my hexo config file.

But some of the ci servers can securely store such info in environment variables, and github is accessible by Personal access token, so I managed to provide the token either from args.token field as plain-text, or a env-var, whose name is from config's token field with a prefix $.

Note that this feature is only functional only if the repo's scheme is http or https, but the test case provide a fake repo from disk. So I only add a stub test case. Do you have any idea to test this new feature?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 9, 2019

Coverage Status

Coverage increased (+1.6%) to 90.58% when pulling 7983b56 on xiaozhikang0916:feature/token into aadcd16 on hexojs:master.

@xiaozhikang0916 xiaozhikang0916 force-pushed the xiaozhikang0916:feature/token branch 4 times, most recently from 3dace4b to 6b3a9e6 Jul 9, 2019
@xiaozhikang0916

This comment has been minimized.

Copy link
Contributor Author

xiaozhikang0916 commented Jul 10, 2019

Implementation and test cases updated, please check.

@xiaozhikang0916 xiaozhikang0916 force-pushed the xiaozhikang0916:feature/token branch from 6b3a9e6 to c0209bf Oct 8, 2019
Copy link
Member

SukkaW left a comment

Consider adding coverge to .gitignore?

@xiaozhikang0916 xiaozhikang0916 force-pushed the xiaozhikang0916:feature/token branch from c0209bf to 5e647a8 Oct 8, 2019
@xiaozhikang0916

This comment has been minimized.

Copy link
Contributor Author

xiaozhikang0916 commented Oct 8, 2019

Consider adding coverge to .gitignore?

Sorry about that, I deleted coverage from history and force-pushed, but did not add it to .gitignore since it is not part of this feature-enhancement.

@xiaozhikang0916 xiaozhikang0916 requested a review from SukkaW Oct 8, 2019
lib/parse_config.js Outdated Show resolved Hide resolved
lib/parse_config.js Outdated Show resolved Hide resolved
test/parse_config.js Outdated Show resolved Hide resolved
@xiaozhikang0916

This comment has been minimized.

Copy link
Contributor Author

xiaozhikang0916 commented Oct 8, 2019

Changes requested have been pushed.

And I think tokens for different repos are hardly the same, and a token config at top level applied to all repoes is about to cause deployment failure. So I move the token config inside each repo's setting.

To achive that, Structured repo setting is supported in this branch, to avoid complicated string parsing. Please check README file for detail.

@xiaozhikang0916 xiaozhikang0916 requested a review from SukkaW Oct 8, 2019
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@xiaozhikang0916 xiaozhikang0916 force-pushed the xiaozhikang0916:feature/token branch from 83f053e to 8855828 Oct 9, 2019
@xiaozhikang0916 xiaozhikang0916 force-pushed the xiaozhikang0916:feature/token branch from 8855828 to 05b3b9b Oct 9, 2019
README.md Outdated Show resolved Hide resolved
@SukkaW
SukkaW approved these changes Oct 9, 2019
@gigaSproule

This comment has been minimized.

Copy link

gigaSproule commented Oct 28, 2019

Is it possible to get this merged in?

@SukkaW

This comment has been minimized.

Copy link
Member

SukkaW commented Oct 28, 2019

@gigaSproule I have approved this Pull Request, but since this Pull Request bring up a major change and we should take careful of it.

@SukkaW SukkaW requested a review from curbengh Oct 28, 2019
@SukkaW SukkaW requested a review from tomap Oct 28, 2019
@gigaSproule

This comment has been minimized.

Copy link

gigaSproule commented Oct 29, 2019

I totally agree, just there wasn't an update from 20 days ago after an approval. I thought that maybe this just got missed off someone's radar.

@xiaozhikang0916

This comment has been minimized.

Copy link
Contributor Author

xiaozhikang0916 commented Oct 29, 2019

@gigaSproule You can make use of this feature from my forked repo before this merge request is accepted, just replace the installation by

npm install https://github.com/xiaozhikang0916/hexo-deployer-git.git --save

It is working fine in my blog repo with github action, check the configuration for detail.

@gigaSproule

This comment has been minimized.

Copy link

gigaSproule commented Oct 29, 2019

@xiaozhikang0916 I'm sure I'm doing something wrong, but it's not working. I tried adding deploy.token and deploy.repo.token, but no luck. Every time remote: Invalid username or password. fatal: Authentication failed for...

Edit: Just been debugging and the npm install you sent was missing the branch, but with that included, it only works with the repo object, not string, which is what I'm guessing is the expected behaviour.

@xiaozhikang0916

This comment has been minimized.

Copy link
Contributor Author

xiaozhikang0916 commented Oct 30, 2019

@gigaSproule Sorry about my mistaken, I thought everything in my feature branch has been merged into my master branch but actually it has not. I just updated it before this comment.

And yes, token config only works with a repo object, but not string. Because it will take grate effort to parse token and/or branch name from a string which is not worthy, and in my opinion, declared every thing you need in a field clearly will be easier to maintain.

README.md Outdated
@@ -49,10 +53,21 @@ deploy:
[another extend directory]: false
ignore_pattern:
[folder]: regexp # or you could specify the ignore_pattern under a certain directory
# keys inside repo do not matter, and can be omitted for single repo with token:

This comment has been minimized.

Copy link
@curbengh

curbengh Nov 2, 2019

Contributor

which keys can be omitted? repo.url & repo.token? Can you give an example config for single repo?

This comment has been minimized.

Copy link
@xiaozhikang0916

xiaozhikang0916 Nov 2, 2019

Author Contributor

It means the name of repo.

For multiple repos config, we have:

deploy:
  repo:
    repo_A: <repository url>[,branch]
    repo_B:
      url: <repository url>
      branch: [branch]
      token: [$ENV_TOKEN]

But for config with single repo, which I think should be the most situation, we will get something like:

deploy:
  repo:
    my_repo:
      url: <repository url>
      branch: [branch]
      token: [$ENV_TOKEN]

In this case, name of repo might be superfluous so we can omit it. Example config for single repo is followed in the next 4 lines.

The description might be misleading so I updated it.

This comment has been minimized.

Copy link
@curbengh

curbengh Nov 3, 2019

Contributor

you mean for a single repo, my_repo: key can be omitted, like

deploy:
  repo:
    url: <repository url>
    branch: [branch]
    token: [$ENV_TOKEN]

it only works with the repo object, not string

so the following wouldn't work?

deploy:
  repo: <repository url>[,branch]

I'm assuming the following also wouldn't work?

deploy:
  repo: 
    my_repo: <repository url>[,branch]

This comment has been minimized.

Copy link
@xiaozhikang0916

xiaozhikang0916 Nov 3, 2019

Author Contributor

Both will work as they are kinds of existing config.

For the first case, check test case 'single repo, branch after url'.

For the following one, the omit of name is optional and it's fine to keep it. This case is updated to the unit test.

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Nov 2, 2019

but since this Pull Request bring up a major change and we should take careful of it.

Is there any breaking change? Does existing config still work? Judging from the readme, I don't think there is any breaking change?

@xiaozhikang0916

This comment has been minimized.

Copy link
Contributor Author

xiaozhikang0916 commented Nov 2, 2019

@curbengh No breaking change here, and existing config is still working, which is guranteed by the unit test. All unit test cases from master are kept here.

@tomap
tomap approved these changes Nov 6, 2019
@tomap tomap merged commit bc3f704 into hexojs:master Nov 6, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.