imbajin commented on code in PR #732:
URL:
https://github.com/apache/hugegraph-toolchain/pull/732#discussion_r3207303835
##########
.github/workflows/loader-ci.yml:
##########
@@ -50,18 +47,16 @@ jobs:
with:
fetch-depth: 2
- - name: Install JDK 11
- uses: actions/setup-java@v4
+ - name: Get HugeGraph stable commit id
+ id: get-commit
+ uses: ./.github/actions/get-hugegraph-commit
Review Comment:
⚠️ **Implicit `$COMMIT_ID` — invisible to readers of this file**
The `Prepare env and service` step (later in the workflow, not in the diff)
still runs:
```sh
$TRAVIS_DIR/install-hugegraph-from-source.sh $COMMIT_ID
```
`COMMIT_ID` is no longer in the job `env` block. It works only because this
composite action calls `core.exportVariable('COMMIT_ID', sha)` which writes to
`$GITHUB_ENV`. A reader scanning the workflow file will see no `COMMIT_ID`
definition and have no idea where it comes from.
All other workflows in this PR pass the SHA explicitly via
`steps.get-commit.outputs.commit_id`. Align loader-ci to the same pattern —
either convert the install step to use the `setup-hugegraph-server` composite
action, or add an explicit `env` binding to that step:
```yaml
- name: Prepare env and service
env:
COMMIT_ID: ${{ steps.get-commit.outputs.commit_id }}
run: |
$TRAVIS_DIR/install-hadoop.sh
$TRAVIS_DIR/install-hugegraph-from-source.sh $COMMIT_ID
```
##########
.github/actions/setup-java-env/action.yml:
##########
@@ -0,0 +1,40 @@
+name: 'Setup Java and Maven Environment'
+description: 'Install JDK, cache Maven packages, and optionally apply staged
Maven repository settings'
+
+inputs:
+ java-version:
+ description: 'Java version to install'
+ required: false
+ default: '11'
+ distribution:
+ description: 'JDK distribution'
+ required: false
+ default: 'zulu'
+ use-stage:
+ description: 'Whether to apply staged Maven repository settings'
+ required: false
+ default: 'true'
+
+runs:
+ using: composite
+ steps:
+ - name: Install JDK ${{ inputs.java-version }}
+ uses: actions/setup-java@v4
+ with:
+ java-version: ${{ inputs.java-version }}
+ distribution: ${{ inputs.distribution }}
+ cache: 'maven'
Review Comment:
⚠️ **Redundant double Maven cache**
`actions/setup-java@v4` with `cache: 'maven'` already saves/restores
`~/.m2/repository` internally using a `setup-java`-namespaced key. The explicit
`Cache Maven packages` step below (lines 28-33) targets the **same directory**
with a different key, so every job saves two separate cache entries for
identical content.
For workflows that previously had only one cache mechanism (`tools-ci`,
`spark-connector-ci`, `client-go-ci`, `loader-ci`), this PR silently doubles
the cache storage and upload time.
Remove either:
- the `cache: 'maven'` line here, keeping the explicit step below, OR
- the entire `- name: Cache Maven packages` block (lines 28-33), relying
solely on `setup-java`'s built-in caching.
Both approaches work; the latter is less YAML and the recommended pattern
for `setup-java@v4`.
##########
hugegraph-client-go/api/v1/gremlin/gemlin.go:
##########
@@ -132,6 +126,19 @@ func (g Post) WithGremlin(gremlin string) func(request
*PostRequest) {
}
}
+func buildDefaultAliases(transport api.Transport) map[string]string {
+ cfg := transport.GetConfig()
+ graphSpace := cfg.GraphSpace
+ if graphSpace == "" {
+ graphSpace = "DEFAULT"
+ }
+ full := graphSpace + "-" + cfg.Graph
Review Comment:
🧹 **Unguarded empty `Graph` field produces a silent bad alias**
If a caller constructs a client without setting `Graph`, `full` becomes
`"DEFAULT-"` and the alias `g` becomes `"__g_DEFAULT-"`. The server will accept
the POST but return an error like `"TraversalSource not found"`, which is very
hard to diagnose.
Since `NewCommonClient` does not validate that `Graph` is non-empty, a small
guard here makes failures explicit:
```suggestion
if cfg.Graph == "" {
cfg.Graph = "hugegraph"
}
full := graphSpace + "-" + cfg.Graph
```
(Or add the check to `NewCommonClient` and document the default.)
##########
.github/workflows/hubble-ci.yml:
##########
@@ -44,33 +41,23 @@ jobs:
with:
fetch-depth: 2
- - name: Install JDK 11
- uses: actions/setup-java@v3
+ - name: Get HugeGraph stable commit id
+ id: get-commit
+ uses: ./.github/actions/get-hugegraph-commit
Review Comment:
⚠️ **Same implicit `$COMMIT_ID` issue as loader-ci**
The `Prepare env and service` step later in this file calls:
```sh
$TRAVIS_DIR/install-hugegraph.sh $COMMIT_ID
```
`steps.get-commit.outputs.commit_id` is never referenced by name anywhere in
this file — it only works via the `core.exportVariable` side-effect. Make the
dependency explicit:
```yaml
- name: Prepare env and service
env:
COMMIT_ID: ${{ steps.get-commit.outputs.commit_id }}
run: |
...
$TRAVIS_DIR/install-hugegraph.sh $COMMIT_ID
```
##########
hugegraph-client-go/api/v1/gremlin/gemlin_test.go:
##########
@@ -31,22 +31,17 @@ func TestGremlin(t *testing.T) {
if err != nil {
log.Println(err)
}
- respGet, err := client.Gremlin.Get(
- client.Gremlin.Get.WithGremlin("hugegraph.traversal().V().limit(3)"),
- )
- if err != nil {
- log.Fatalln(err)
- }
- if respGet.StatusCode != 200 {
- t.Error("client.Gremlin.GremlinGet error ")
- }
respPost, err := client.Gremlin.Post(
- client.Gremlin.Post.WithGremlin("hugegraph.traversal().V().limit(3)"),
+ client.Gremlin.Post.WithGremlin("g.V().limit(3)"),
)
if err != nil {
log.Fatalln(err)
}
+ if respPost.StatusCode != 200 {
+ t.Errorf("client.Gremlin.Post http_status=%d, gremlin_status=%d,
message=%s",
Review Comment:
🧹 **Extra leading space — fails `gofmt`**
The `t.Errorf` line has 5 spaces of indentation instead of a tab. `gofmt`
will produce a diff on this file.
```suggestion
t.Errorf("client.Gremlin.Post http_status=%d,
gremlin_status=%d, message=%s",
respPost.StatusCode, respPost.Data.Status.Code,
respPost.Data.Status.Message)
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
