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

chore(go-client): add generation thrift files of go-client #1917

Merged
merged 9 commits into from
Jun 20, 2024

Conversation

lengyuexuexuan
Copy link
Collaborator

What problem does this PR solve?

#1881

What is changed and how does it work?

By uploading generation thrift files, the go client can be used directly by users through "go get" without the need to compile it locally.

Tests
  • Manual test (add detailed scripts or steps below)

@acelyc111
Copy link
Member

@lengyuexuexuan Please check the failed CIs, for Check License, you need to add the new files to .licenserc.yaml.

@acelyc111
Copy link
Member

Now that the go files have been pre-generated, the generate steps in https://github.com/apache/incubator-pegasus/blob/master/go-client/Makefile#L19 can be removed, right? Then you need to add the manual generate steps in go-client/README.md.

And more, you need to add steps in .github/workflows/lint_and_test_go-client.yml to check that the pre-generated go files are the same to the ones generated manually.

@lengyuexuexuan
Copy link
Collaborator Author

Now that the go files have been pre-generated, the generate steps in https://github.com/apache/incubator-pegasus/blob/master/go-client/Makefile#L19 can be removed, right? Then you need to add the manual generate steps in go-client/README.md.

And more, you need to add steps in .github/workflows/lint_and_test_go-client.yml to check that the pre-generated go files are the same to the ones generated manually.

Is it possible to remove the thrift file directly in the .gitignore file, so that if the thrift file is modified, it can be uploaded directly?

@acelyc111
Copy link
Member

Now that the go files have been pre-generated, the generate steps in https://github.com/apache/incubator-pegasus/blob/master/go-client/Makefile#L19 can be removed, right? Then you need to add the manual generate steps in go-client/README.md.
And more, you need to add steps in .github/workflows/lint_and_test_go-client.yml to check that the pre-generated go files are the same to the ones generated manually.

Is it possible to remove the thrift file directly in the .gitignore file, so that if the thrift file is modified, it can be uploaded directly?

Of course, feel free to do that.

acelyc111
acelyc111 previously approved these changes May 23, 2024
@acelyc111
Copy link
Member

@lengyuexuexuan Do you have some time to improve the PR ? I'm pleased to give you some help if needed.

@lengyuexuexuan
Copy link
Collaborator Author

@lengyuexuexuan Do you have some time to improve the PR ? I'm pleased to give you some help if needed.

thanks. Today or Tomorrow I will improve this pr.

@lengyuexuexuan
Copy link
Collaborator Author

@acelyc111
Now I have uploaded the latest version of the thrift file, and remove go-client/idl/* in .gitignored file.
I tested it and now users can directly pull the latest version of go client and use it directly.
It needs another committer to aprrove this pr and #1916.

@acelyc111
Copy link
Member

according to the error, you need to add these file names [1] to .licenserc.yaml (add them in property place to keep in dictionary order).

  1. https://github.com/apache/incubator-pegasus/actions/runs/9285106967/job/25548898496?pr=1917#step:3:141

@acelyc111
Copy link
Member

@lengyuexuexuan
Copy link
Collaborator Author

.gitignore Outdated Show resolved Hide resolved
.licenserc.yaml Outdated Show resolved Hide resolved
@empiredan empiredan merged commit 44e8350 into apache:master Jun 20, 2024
15 of 16 checks passed
ruojieranyishen pushed a commit to ruojieranyishen/incubator-pegasus that referenced this pull request Jul 17, 2024
apache#1881

By uploading generation thrift files, the go client can be used directly by users through "go get" without the need to compile it locally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants