You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "diaohancai (via GitHub)" <gi...@apache.org> on 2023/09/15 02:08:36 UTC
[GitHub] [incubator-hugegraph-computer] diaohancai opened a new pull request, #265: fix: hugegraph client authentication configuration
diaohancai opened a new pull request, #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265
<!--
Thank you very much for contributing to Apache HugeGraph, we are happy that you want to help us improve it!
Here are some tips for you:
1. If this is your first time, please read the [contributing guidelines](https://github.com/apache/hugegraph/blob/master/CONTRIBUTING.md)
2. If a PR will fix/close a issue, type the message "close xxx" (xxx is the link of related issue) in the content, github will auto link it (Required)
3. Name the PR title in "Google Commit Format", start with "feat | fix | perf | refactor | doc | chore",
such like: "feat(core): support the PageRank algorithm" or "fix: wrong break in the compute loop" (module is optional)
skip it if you are unsure about which is the best component.
4. One PR address one issue, better not to mix up multiple issues.
5. Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]` (or click it directly after published)
-->
## Purpose of the PR
- fix #264 <!-- or use "fix #xxx", "xxx" is the ID-link of related issue, e.g: close #257 -->
<!--
Please explain more context in this section, clarify why the changes are needed.
For example:
- If you propose a new API, clarify the use case for a new API.
- If you fix a bug, you can clarify why it is a bug, and should associated with an issue.
-->
## Main Changes
<!-- Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue. These change logs are helpful for better ant faster reviews.)
For example:
- If you introduce a new feature, please show detailed design here or add the link of design documentation.
- If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
- If there is a discussion in the mailing list, please add the link. -->
- Add authentication configuration.
computer.properties:
`hugegraph.username=`
`hugegraph.password=`
- Construct HugeClient add configUser:
`new HugeClientBuilder(url, graph).configUser(username, password)`
## Verifying these changes
<!-- Please pick the proper options below -->
- [X] Trivial rework / code cleanup without any test coverage. (No Need)
- [ ] Already covered by existing tests, such as *(please modify tests here)*.
- [ ] Need tests and can be verified as follows.
<!-- Please provide more details about verification
For example:
- If you test manually, please provide related screenshot.
-->
## Does this PR potentially affect the following parts?
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [X] Nope
- [ ] Dependencies (add/update license info) <!-- Don't forget to add/update the info in "LICENSE" & "NOTICE" files (both in root & dist module) -->
- [ ] Modify configurations
- [ ] The public API
- [ ] Other affects (typed here)
## Documentation Status
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `Doc - TODO` <!-- Your PR changes impact docs and you will update later -->
- [ ] `Doc - Done` <!-- Related docs have been already added or updated -->
- [X] `Doc - No Need` <!-- Your PR changes don't impact/need docs -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] diaohancai commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "diaohancai (via GitHub)" <gi...@apache.org>.
diaohancai commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1327919253
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/output/hg/task/TaskManager.java:
##########
@@ -56,7 +56,9 @@ public TaskManager(Config config) {
this.config = config;
String url = config.get(ComputerOptions.HUGEGRAPH_URL);
String graph = config.get(ComputerOptions.HUGEGRAPH_GRAPH_NAME);
- this.client = new HugeClientBuilder(url, graph).build();
+ String username = config.get(ComputerOptions.HUGEGRAPH_USERNAME);
+ String password = config.get(ComputerOptions.HUGEGRAPH_PASSWORD);
+ this.client = new HugeClientBuilder(url, graph).configUser(username, password).build();
Review Comment:
Hi~ I reviewed the code. Now I'm confused about two things.
1. HugeClientBuilder api not found
`HugeClientBuilder.configToken(token)`
I did not found this api, should I upgrade the version of 'hugegraph-client' dependency?
2. Auth mode choose
[hugegraph user's guide config-authentication](https://hugegraph.apache.org/cn/docs/config/config-authentication/)
As hugegraph user's guide said, there are two auth mode:
- ConfigAuthenticator: configure user and token in rest-server.properties statically.
- StandardAuthenticator: configure user and password in DB dynamically.
And The 'ConfigAuthenticator mode' seems not to be recommended, is it same as `configToken(token)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] imbajin commented on a diff in pull request #265: fix: hugegraph client authentication configuration
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1326733806
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/output/hg/task/TaskManager.java:
##########
@@ -56,7 +56,9 @@ public TaskManager(Config config) {
this.config = config;
String url = config.get(ComputerOptions.HUGEGRAPH_URL);
String graph = config.get(ComputerOptions.HUGEGRAPH_GRAPH_NAME);
- this.client = new HugeClientBuilder(url, graph).build();
+ String username = config.get(ComputerOptions.HUGEGRAPH_USERNAME);
+ String password = config.get(ComputerOptions.HUGEGRAPH_PASSWORD);
+ this.client = new HugeClientBuilder(url, graph).configUser(username, password).build();
Review Comment:
Some refer:
```java
String token = config.get(ComputerOptions.AUTH_TOKEN);
String usrname = config.get(ComputerOptions.AUTH_USRNAME);
String passwd = config.get(ComputerOptions.AUTH_PASSWD);
// Use token first, then try passwd mode
HugeClientBuilder clientBuilder = new HugeClientBuilder(url, graph);
if (token != null && token.length() != 0) {
this.client = clientBuilder.configToken(token).build();
} else if (usrname != null && usrname.length() != 0) {
this.client = clientBuilder.configUser(usrname, passwd).build();
} else {
this.client = clientBuilder.build();
}
```
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/config/ComputerOptions.java:
##########
@@ -645,6 +645,22 @@ public static synchronized ComputerOptions instance() {
"hugegraph"
);
+ public static final ConfigOption<String> HUGEGRAPH_USERNAME =
+ new ConfigOption<>(
+ "hugegraph.username",
+ "The graph username and password used for authentication.",
+ null,
+ ""
+ );
+
+ public static final ConfigOption<String> HUGEGRAPH_PASSWORD =
+ new ConfigOption<>(
+ "hugegraph.password",
+ "The graph username and password used for authentication.",
+ null,
+ ""
+ );
+
Review Comment:
Some old refer:
```java
public static final ConfigOption<String> AUTH_TOKEN =
new ConfigOption<>(
"hugegraph.token",
"The auth value for compute job to certificate," +
" should only used for HugeGraph server now",
""
);
public static final ConfigOption<String> AUTH_USRNAME =
new ConfigOption<>(
"hugegraph.usrname",
"The usrname for compute job to certificate with " +
"basic auth, should only used in test environment, " +
"consider ban it in future.",
""
);
public static final ConfigOption<String> AUTH_PASSWD =
new ConfigOption<>(
"hugegraph.passwd",
"The password for compute job to certificate with " +
"basic auth, should only used in test environment, " +
"consider ban it in future.",
""
);
```
##########
computer-test/src/main/java/org/apache/hugegraph/computer/suite/unit/UnitTestBase.java:
##########
@@ -127,6 +129,11 @@ public static void init() throws ClassNotFoundException {
GRAPH = ComputerOptions.HUGEGRAPH_GRAPH_NAME
.defaultValue();
+ USERNAME = ComputerOptions.HUGEGRAPH_USERNAME
+ .defaultValue();
+ PASSWORD = ComputerOptions.HUGEGRAPH_PASSWORD
+ .defaultValue();
+
Review Comment:
could refer:
<img width="757" alt="image" src="https://github.com/apache/incubator-hugegraph-computer/assets/17706099/8620ea0d-8d3f-4605-a64f-cfd5d747339c">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] diaohancai commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "diaohancai (via GitHub)" <gi...@apache.org>.
diaohancai commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1327919253
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/output/hg/task/TaskManager.java:
##########
@@ -56,7 +56,9 @@ public TaskManager(Config config) {
this.config = config;
String url = config.get(ComputerOptions.HUGEGRAPH_URL);
String graph = config.get(ComputerOptions.HUGEGRAPH_GRAPH_NAME);
- this.client = new HugeClientBuilder(url, graph).build();
+ String username = config.get(ComputerOptions.HUGEGRAPH_USERNAME);
+ String password = config.get(ComputerOptions.HUGEGRAPH_PASSWORD);
+ this.client = new HugeClientBuilder(url, graph).configUser(username, password).build();
Review Comment:
Hi~ I reviewed the code. Now I'm confused about two things.
1. HugeClientBuilder api not found
`HugeClientBuilder.configToken(token)`
I did not found this api, should I upgrade the version of 'hugegraph-client' dependency?
2. Auth mode choose
[hugegraph user's guide config-authentication](https://hugegraph.apache.org/cn/docs/config/config-authentication/)
As hugegraph user's guide said, there are two auth mode:
- ConfigAuthenticator: configure user and token in rest-server.properties statically.
- StandardAuthenticator: configure user and password in DB dynamically.
And The 'ConfigAuthenticator mode' seems not to be recommended, is it same as `configToken(token)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] imbajin commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1329553847
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/output/hg/task/TaskManager.java:
##########
@@ -56,7 +56,9 @@ public TaskManager(Config config) {
this.config = config;
String url = config.get(ComputerOptions.HUGEGRAPH_URL);
String graph = config.get(ComputerOptions.HUGEGRAPH_GRAPH_NAME);
- this.client = new HugeClientBuilder(url, graph).build();
+ String username = config.get(ComputerOptions.HUGEGRAPH_USERNAME);
+ String password = config.get(ComputerOptions.HUGEGRAPH_PASSWORD);
+ this.client = new HugeClientBuilder(url, graph).configUser(username, password).build();
Review Comment:
> @diaohancai @imbajin seems `configToken` has deprecated in hugegraph-client-1.0.0, we can use `configUser` instead. refer to [apache/incubator-hugegraph-toolchain@`1.0.0`/hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClientBuilder.java#L108](https://github.com/apache/incubator-hugegraph-toolchain/blob/1.0.0/hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClientBuilder.java?rgh-link-date=2023-09-19T03%3A35%3A21Z#L108)
OK, the refer code is also outdated:)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refact(core): support auth config for computer task [incubator-hugegraph-computer]
Posted by "diaohancai (via GitHub)" <gi...@apache.org>.
diaohancai commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1349510829
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/config/ComputerOptions.java:
##########
@@ -645,6 +645,22 @@ public static synchronized ComputerOptions instance() {
"hugegraph"
);
+ public static final ConfigOption<String> HUGEGRAPH_USERNAME =
+ new ConfigOption<>(
+ "hugegraph.username",
+ "The graph username and password used for authentication.",
Review Comment:
ok~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refact(core): support auth config for computer task [incubator-hugegraph-computer]
Posted by "diaohancai (via GitHub)" <gi...@apache.org>.
diaohancai commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1349511855
##########
computer-test/src/main/java/org/apache/hugegraph/computer/suite/unit/UnitTestBase.java:
##########
@@ -127,6 +129,11 @@ public static void init() throws ClassNotFoundException {
GRAPH = ComputerOptions.HUGEGRAPH_GRAPH_NAME
.defaultValue();
+ USERNAME = ComputerOptions.HUGEGRAPH_USERNAME
+ .defaultValue();
+ PASSWORD = ComputerOptions.HUGEGRAPH_PASSWORD
+ .defaultValue();
Review Comment:
ok~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refact(core): support auth config for computer task [incubator-hugegraph-computer]
Posted by "simon824 (via GitHub)" <gi...@apache.org>.
simon824 merged PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] diaohancai commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "diaohancai (via GitHub)" <gi...@apache.org>.
diaohancai commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1334005936
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/output/hg/task/TaskManager.java:
##########
@@ -56,7 +56,9 @@ public TaskManager(Config config) {
this.config = config;
String url = config.get(ComputerOptions.HUGEGRAPH_URL);
String graph = config.get(ComputerOptions.HUGEGRAPH_GRAPH_NAME);
- this.client = new HugeClientBuilder(url, graph).build();
+ String username = config.get(ComputerOptions.HUGEGRAPH_USERNAME);
+ String password = config.get(ComputerOptions.HUGEGRAPH_PASSWORD);
+ this.client = new HugeClientBuilder(url, graph).configUser(username, password).build();
Review Comment:
Hi~ Is there anything needs to be improved this PR? @imbajin
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] imbajin commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1328048728
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/output/hg/task/TaskManager.java:
##########
@@ -56,7 +56,9 @@ public TaskManager(Config config) {
this.config = config;
String url = config.get(ComputerOptions.HUGEGRAPH_URL);
String graph = config.get(ComputerOptions.HUGEGRAPH_GRAPH_NAME);
- this.client = new HugeClientBuilder(url, graph).build();
+ String username = config.get(ComputerOptions.HUGEGRAPH_USERNAME);
+ String password = config.get(ComputerOptions.HUGEGRAPH_PASSWORD);
+ this.client = new HugeClientBuilder(url, graph).configUser(username, password).build();
Review Comment:
> Hi~ I reviewed the code. Now I'm confused about two things.
>
> 1. HugeClientBuilder api not found
> `HugeClientBuilder.configToken(token)`
> I did not found this api, should I upgrade the version of 'hugegraph-client' dependency?
>
> 2. Auth mode choose
> [hugegraph user's guide config-authentication](https://hugegraph.apache.org/cn/docs/config/config-authentication/)
> As hugegraph user's guide said, there are two auth mode:
> - ConfigAuthenticator mode: configure user and token in rest-server.properties statically.
> - StandardAuthenticator mode: configure user and password in DB dynamically.
> And the ConfigAuthenticator mode seems not to be recommended, is it same as `HugeClientBuilder.configToken(token)`?
I'll check the code & doc soon?(maybe the doc/client is outdated)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] simon824 commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "simon824 (via GitHub)" <gi...@apache.org>.
simon824 commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1329539323
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/output/hg/task/TaskManager.java:
##########
@@ -56,7 +56,9 @@ public TaskManager(Config config) {
this.config = config;
String url = config.get(ComputerOptions.HUGEGRAPH_URL);
String graph = config.get(ComputerOptions.HUGEGRAPH_GRAPH_NAME);
- this.client = new HugeClientBuilder(url, graph).build();
+ String username = config.get(ComputerOptions.HUGEGRAPH_USERNAME);
+ String password = config.get(ComputerOptions.HUGEGRAPH_PASSWORD);
+ this.client = new HugeClientBuilder(url, graph).configUser(username, password).build();
Review Comment:
@diaohancai @imbajin
seems `configToken` has deprecated in hugegraph-client-1.0.0, we can use `configUser` instead.
refer to https://github.com/apache/incubator-hugegraph-toolchain/blob/1.0.0/hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClientBuilder.java#L108
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] simon824 commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "simon824 (via GitHub)" <gi...@apache.org>.
simon824 commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1334014573
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/config/ComputerOptions.java:
##########
@@ -645,6 +645,22 @@ public static synchronized ComputerOptions instance() {
"hugegraph"
);
+ public static final ConfigOption<String> HUGEGRAPH_USERNAME =
+ new ConfigOption<>(
+ "hugegraph.username",
+ "The graph username and password used for authentication.",
+ null,
+ ""
+ );
+
+ public static final ConfigOption<String> HUGEGRAPH_PASSWORD =
+ new ConfigOption<>(
+ "hugegraph.password",
+ "The graph username and password used for authentication.",
Review Comment:
ditto
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/config/ComputerOptions.java:
##########
@@ -645,6 +645,22 @@ public static synchronized ComputerOptions instance() {
"hugegraph"
);
+ public static final ConfigOption<String> HUGEGRAPH_USERNAME =
+ new ConfigOption<>(
+ "hugegraph.username",
+ "The graph username and password used for authentication.",
Review Comment:
improve description
```suggestion
"The username of graph for authentication.",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] diaohancai commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "diaohancai (via GitHub)" <gi...@apache.org>.
diaohancai commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1327919253
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/output/hg/task/TaskManager.java:
##########
@@ -56,7 +56,9 @@ public TaskManager(Config config) {
this.config = config;
String url = config.get(ComputerOptions.HUGEGRAPH_URL);
String graph = config.get(ComputerOptions.HUGEGRAPH_GRAPH_NAME);
- this.client = new HugeClientBuilder(url, graph).build();
+ String username = config.get(ComputerOptions.HUGEGRAPH_USERNAME);
+ String password = config.get(ComputerOptions.HUGEGRAPH_PASSWORD);
+ this.client = new HugeClientBuilder(url, graph).configUser(username, password).build();
Review Comment:
Hi~ I reviewed the code. Now I'm confused about two things.
1. HugeClientBuilder api not found
`HugeClientBuilder.configToken(token)`
I did not found this api, should I upgrade the version of 'hugegraph-client' dependency?
2. Auth mode choose
[hugegraph user's guide config-authentication](https://hugegraph.apache.org/cn/docs/config/config-authentication/)
As hugegraph user's guide said, there are two auth mode:
- ConfigAuthenticator mode: configure user and token in rest-server.properties statically.
- StandardAuthenticator mode: configure user and password in DB dynamically.
And The 'ConfigAuthenticator mode' seems not to be recommended, is it same as `configToken(token)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refact(core): support auth config for computer task [incubator-hugegraph-computer]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#issuecomment-1751743287
## [Codecov](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/265?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#265](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/265?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (db71320) into [master](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/commit/fdb4621330d76d2d74f5162864fff28365fa735f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (fdb4621) will **increase** coverage by `0.00%`.
> Report is 2 commits behind head on master.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #265 +/- ##
=========================================
Coverage 85.83% 85.83%
+ Complexity 3235 3234 -1
=========================================
Files 344 344
Lines 12115 12124 +9
Branches 1092 1092
=========================================
+ Hits 10399 10407 +8
Misses 1194 1194
- Partials 522 523 +1
```
| [Files](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/265?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ugegraph/computer/core/config/ComputerOptions.java](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/265?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tcHV0ZXItYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWdlZ3JhcGgvY29tcHV0ZXIvY29yZS9jb25maWcvQ29tcHV0ZXJPcHRpb25zLmphdmE=) | `98.90% <100.00%> (+<0.01%)` | :arrow_up: |
| [...raph/computer/core/output/hg/task/TaskManager.java](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/265?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tcHV0ZXItYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWdlZ3JhcGgvY29tcHV0ZXIvY29yZS9vdXRwdXQvaGcvdGFzay9UYXNrTWFuYWdlci5qYXZh) | `63.51% <100.00%> (+1.01%)` | :arrow_up: |
| [...graph/computer/core/input/hg/HugeGraphFetcher.java](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/265?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tcHV0ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL2NvbXB1dGVyL2NvcmUvaW5wdXQvaGcvSHVnZUdyYXBoRmV0Y2hlci5qYXZh) | `100.00% <100.00%> (ø)` | |
| [.../computer/core/input/hg/HugeInputSplitFetcher.java](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/265?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tcHV0ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL2NvbXB1dGVyL2NvcmUvaW5wdXQvaGcvSHVnZUlucHV0U3BsaXRGZXRjaGVyLmphdmE=) | `93.93% <100.00%> (+0.60%)` | :arrow_up: |
... and [2 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/265/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] imbajin commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1326733499
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/config/ComputerOptions.java:
##########
@@ -645,6 +645,22 @@ public static synchronized ComputerOptions instance() {
"hugegraph"
);
+ public static final ConfigOption<String> HUGEGRAPH_USERNAME =
+ new ConfigOption<>(
+ "hugegraph.username",
+ "The graph username and password used for authentication.",
+ null,
+ ""
+ );
+
+ public static final ConfigOption<String> HUGEGRAPH_PASSWORD =
+ new ConfigOption<>(
+ "hugegraph.password",
+ "The graph username and password used for authentication.",
+ null,
+ ""
+ );
+
Review Comment:
Some old refer: (Note: the `description/variable name` **could be improved**, no need to **copy it**)
```java
public static final ConfigOption<String> AUTH_TOKEN =
new ConfigOption<>(
"hugegraph.token",
"The auth value for compute job to certificate," +
" should only used for HugeGraph server now",
""
);
public static final ConfigOption<String> AUTH_USRNAME =
new ConfigOption<>(
"hugegraph.usrname",
"The usrname for compute job to certificate with " +
"basic auth, should only used in test environment, " +
"consider ban it in future.",
""
);
public static final ConfigOption<String> AUTH_PASSWD =
new ConfigOption<>(
"hugegraph.passwd",
"The password for compute job to certificate with " +
"basic auth, should only used in test environment, " +
"consider ban it in future.",
""
);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] diaohancai commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "diaohancai (via GitHub)" <gi...@apache.org>.
diaohancai commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1327919253
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/output/hg/task/TaskManager.java:
##########
@@ -56,7 +56,9 @@ public TaskManager(Config config) {
this.config = config;
String url = config.get(ComputerOptions.HUGEGRAPH_URL);
String graph = config.get(ComputerOptions.HUGEGRAPH_GRAPH_NAME);
- this.client = new HugeClientBuilder(url, graph).build();
+ String username = config.get(ComputerOptions.HUGEGRAPH_USERNAME);
+ String password = config.get(ComputerOptions.HUGEGRAPH_PASSWORD);
+ this.client = new HugeClientBuilder(url, graph).configUser(username, password).build();
Review Comment:
Hi~ I reviewed the code. Now I'm confused about two things.
1. HugeClientBuilder api not found
`HugeClientBuilder.configToken(token)`
I did not found this api, should I upgrade the version of 'hugegraph-client' dependency?
2. Auth mode choose
[hugegraph user's guide config-authentication](https://hugegraph.apache.org/cn/docs/config/config-authentication/)
As hugegraph user's guide said, there are two auth mode:
- ConfigAuthenticator mode: configure user and token in rest-server.properties statically.
- StandardAuthenticator mode: configure user and password in DB dynamically.
And the ConfigAuthenticator mode seems not to be recommended, is it same as `configToken(token)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] javeme commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1342001054
##########
computer-test/src/main/java/org/apache/hugegraph/computer/suite/unit/UnitTestBase.java:
##########
@@ -127,6 +129,11 @@ public static void init() throws ClassNotFoundException {
GRAPH = ComputerOptions.HUGEGRAPH_GRAPH_NAME
.defaultValue();
+ USERNAME = ComputerOptions.HUGEGRAPH_USERNAME
+ .defaultValue();
+ PASSWORD = ComputerOptions.HUGEGRAPH_PASSWORD
+ .defaultValue();
Review Comment:
can we put them into a line, the same to URL and GRAPH
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] imbajin commented on a diff in pull request #265: fix: hugegraph client authentication configuration
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1326733499
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/config/ComputerOptions.java:
##########
@@ -645,6 +645,22 @@ public static synchronized ComputerOptions instance() {
"hugegraph"
);
+ public static final ConfigOption<String> HUGEGRAPH_USERNAME =
+ new ConfigOption<>(
+ "hugegraph.username",
+ "The graph username and password used for authentication.",
+ null,
+ ""
+ );
+
+ public static final ConfigOption<String> HUGEGRAPH_PASSWORD =
+ new ConfigOption<>(
+ "hugegraph.password",
+ "The graph username and password used for authentication.",
+ null,
+ ""
+ );
+
Review Comment:
Some old refer: (Note: the description could be improved, no need to copy it)
```java
public static final ConfigOption<String> AUTH_TOKEN =
new ConfigOption<>(
"hugegraph.token",
"The auth value for compute job to certificate," +
" should only used for HugeGraph server now",
""
);
public static final ConfigOption<String> AUTH_USRNAME =
new ConfigOption<>(
"hugegraph.usrname",
"The usrname for compute job to certificate with " +
"basic auth, should only used in test environment, " +
"consider ban it in future.",
""
);
public static final ConfigOption<String> AUTH_PASSWD =
new ConfigOption<>(
"hugegraph.passwd",
"The password for compute job to certificate with " +
"basic auth, should only used in test environment, " +
"consider ban it in future.",
""
);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] diaohancai commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "diaohancai (via GitHub)" <gi...@apache.org>.
diaohancai commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1327919253
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/output/hg/task/TaskManager.java:
##########
@@ -56,7 +56,9 @@ public TaskManager(Config config) {
this.config = config;
String url = config.get(ComputerOptions.HUGEGRAPH_URL);
String graph = config.get(ComputerOptions.HUGEGRAPH_GRAPH_NAME);
- this.client = new HugeClientBuilder(url, graph).build();
+ String username = config.get(ComputerOptions.HUGEGRAPH_USERNAME);
+ String password = config.get(ComputerOptions.HUGEGRAPH_PASSWORD);
+ this.client = new HugeClientBuilder(url, graph).configUser(username, password).build();
Review Comment:
Hi~ I reviewed the code. Now I'm confused about two things.
1. HugeClientBuilder api not found
`HugeClientBuilder.configToken(token)`
I did not found this api, should I upgrade the version of 'hugegraph-client' dependency?
2. Auth mode choose
[hugegraph user's guide config-authentication](https://hugegraph.apache.org/cn/docs/config/config-authentication/)
As hugegraph user's guide said, there are two auth mode:
- ConfigAuthenticator mode: configure user and token in rest-server.properties statically.
- StandardAuthenticator mode: configure user and password in DB dynamically.
And the ConfigAuthenticator mode seems not to be recommended, is it same as `HugeClientBuilder.configToken(token)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] diaohancai commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "diaohancai (via GitHub)" <gi...@apache.org>.
diaohancai commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1326881432
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/config/ComputerOptions.java:
##########
@@ -645,6 +645,22 @@ public static synchronized ComputerOptions instance() {
"hugegraph"
);
+ public static final ConfigOption<String> HUGEGRAPH_USERNAME =
+ new ConfigOption<>(
+ "hugegraph.username",
+ "The graph username and password used for authentication.",
+ null,
+ ""
+ );
+
+ public static final ConfigOption<String> HUGEGRAPH_PASSWORD =
+ new ConfigOption<>(
+ "hugegraph.password",
+ "The graph username and password used for authentication.",
+ null,
+ ""
+ );
+
Review Comment:
Thank you~ Nice refer~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-computer] imbajin commented on a diff in pull request #265: refact(core): support auth config for computer task
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #265:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/265#discussion_r1335296574
##########
computer-api/src/main/java/org/apache/hugegraph/computer/core/output/hg/task/TaskManager.java:
##########
@@ -56,7 +56,9 @@ public TaskManager(Config config) {
this.config = config;
String url = config.get(ComputerOptions.HUGEGRAPH_URL);
String graph = config.get(ComputerOptions.HUGEGRAPH_GRAPH_NAME);
- this.client = new HugeClientBuilder(url, graph).build();
+ String username = config.get(ComputerOptions.HUGEGRAPH_USERNAME);
+ String password = config.get(ComputerOptions.HUGEGRAPH_PASSWORD);
+ this.client = new HugeClientBuilder(url, graph).configUser(username, password).build();
Review Comment:
> Hi~ Is there anything needs to be improved this PR? @imbajin
thanks，wait for another reviewer @javeme @coderzc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org