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

feat: update common version and remove jersey code #538

Merged
merged 60 commits into from
Dec 5, 2023

Conversation

zhenyuT
Copy link
Contributor

@zhenyuT zhenyuT commented Nov 23, 2023

Purpose of the PR

之前提过一个draft PR(已关闭):

直接在client中将jersey相关代码用okhttp替换掉。

对应的issue见:

后面将对应的实现在common中进行实现,并提交了PR(已合入主分支):

所以现在需要对toochain的代码进行整改,升级common版本,并进行代码适配,移除jersey相关的代码

Main Changes

Verifying these changes

  • 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:
    • xxx

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API
  • Other affects (typed here)

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

Great improvement, hopefully we can get rid of jersey's issues from now on.

@@ -23,7 +24,7 @@ on:
env:
TRAVIS_DIR: hugegraph-hubble/hubble-dist/assembly/travis
# TODO: need update it later (eed6103359fe40d2f1476fb8c56d9388c3111a99)
COMMIT_ID: be6ee386b9939dc6bd6fcbdf2274b8acc3a0a314
COMMIT_ID: 69e6b461add1051831dd6cc0c36a5e249a3b3176
Copy link
Contributor

Choose a reason for hiding this comment

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

@imbajin can we share the same COMMIT_ID with client-ci.yml (just use an global env var)

Copy link
Member

@imbajin imbajin Dec 4, 2023

Choose a reason for hiding this comment

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

@imbajin can we share the same COMMIT_ID with client-ci.yml (just use an global env var)

I do want it too but not found the way to achieve it, however I get a better way to avoid hard code commit ID in ci

we could use "latestCommitID - X" (X >= 15 maybe) to get the stable code in server & no need update the ID by ourselves if not necessary (will submit a PR after this adaptor) #542

Copy link
Contributor

@javeme javeme Dec 5, 2023

Choose a reason for hiding this comment

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

It is a method to use latestCommitID - X, but we can't control it freely. If we want to adapt to the latest code, we still need to change the X.
we may try reusable-workflow: https://docs.github.com/en/actions/using-workflows/reusing-workflows#using-inputs-and-secrets-in-a-reusable-workflow

Copy link
Member

@imbajin imbajin Dec 5, 2023

Choose a reason for hiding this comment

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

It is a method to use latestCommitID - X, but we can't control it freely. If we want to adapt to the latest code, we still need to change the X. we may try reusable-workflow: docs.github.com/en/actions/using-workflows/reusing-workflows#using-inputs-and-secrets-in-a-reusable-workflow

Get it, but we can't set secrets/variables in settings, we lack the permission to set it 😢 (This is also the reason why in the early code, we could only write fixed commitID in multiple CI files)

BTW, we could create/submit a common action and publish it to github action repo & let other actions to extend it if github supports

@github-actions github-actions bot added the tools hugegraph-tools label Dec 4, 2023
@github-actions github-actions bot removed the tools hugegraph-tools label Dec 5, 2023
Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

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

LGTM

@simon824 simon824 merged commit cb68bea into apache:master Dec 5, 2023
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client hugegraph-client hubble hugegraph-hubble loader hugegraph-loader spark
Projects
Status: Done
4 participants