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:add support goframe v2 #203

Merged
merged 22 commits into from
Sep 20, 2024
Merged

feat:add support goframe v2 #203

merged 22 commits into from
Sep 20, 2024

Conversation

huicunjun
Copy link
Contributor

add support goframe v2

goframe link:https://github.com/gogf/gf

@mrproliu mrproliu self-requested a review September 18, 2024 10:05
@mrproliu mrproliu added this to the 0.6.0 milestone Sep 18, 2024
Copy link
Contributor

@mrproliu mrproliu left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, after the comment issues, there still has two things needs to be updated:

  1. Please update the [CHANGES.md] to announcement there have a new plugin enhanced.
  2. Please add a Plugin test to checking the plugin is work.

plugins/goframe/go.mod Outdated Show resolved Hide resolved
plugins/goframe/instrument.go Show resolved Hide resolved
plugins/goframe/instrument.go Outdated Show resolved Hide resolved
plugins/goframe/net/ghttp/intercepter.go Show resolved Hide resolved
plugins/goframe/net/ghttp/intercepter.go Outdated Show resolved Hide resolved
plugins/goframe/net/ghttp/intercepter.go Outdated Show resolved Hide resolved
@huicunjun
Copy link
Contributor Author

Good morning, I have fixed all the problems you reviewed, you can have a look at my latest submission, can it be merged into it?

The latest submission is here.
see 1f4a08b

Copy link
Contributor

@mrproliu mrproliu left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, please fix the CI and comments.

CHANGES.md Outdated Show resolved Hide resolved
go.work Outdated Show resolved Hide resolved
plugins/goframe/instrument.go Outdated Show resolved Hide resolved
plugins/goframe/net/ghttp/intercepter.go Outdated Show resolved Hide resolved
test/plugins/scenarios/goframe/config/excepted.yml Outdated Show resolved Hide resolved
test/plugins/scenarios/goframe/main.go Outdated Show resolved Hide resolved
test/plugins/scenarios/goframe/main.go Show resolved Hide resolved
test/plugins/scenarios/goframe/plugin.yml Show resolved Hide resolved
@huicunjun
Copy link
Contributor Author

Thank for the review, I have already fix the CI and comments.

@huicunjun
Copy link
Contributor Author

There seems to be some issues that were not caused by the plugin I submitted

what can I do for this

@wu-sheng
Copy link
Member

There seems to be some issues that were not caused by the plugin I submitted

what can I do for this

The comments were made, your code changes affect the lint check.

@wu-sheng
Copy link
Member

Lint should be easy to check locally. Please make sure it passed before we cost too much CI resources.

@huicunjun
Copy link
Contributor Author

Lint should be easy to check locally. Please make sure it passed before we cost too much CI resources.

sorry ,Because I haven't installed Lint locally

I have processed my plugin part,

but it seems that some of the code for other modules in the project has not been validated by Lint

@wu-sheng
Copy link
Member

All codes merged must pass lint check. so, all fails are related to your codes.

@huicunjun
Copy link
Contributor Author

All codes merged must pass lint check. so, all fails are related to your codes.

image
image

this code is fork The original project ,Can I modify it?
I want to add "//nolint " comment to these failed files

@wu-sheng
Copy link
Member

Are you using the correct version of go lint? Different versions could result in different style details.

@huicunjun
Copy link
Contributor Author

All codes merged must pass lint check. so, all fails are related to your codes.

image image

this code is fork The original project ,Can I modify it? I want to add "//nolint " comment to these failed files

I think if I make changes to these, the original author @mrproliu won't approve them for me

@wu-sheng
Copy link
Member

We should not. The CI should be able to pass for no change code in the main branch. If your local setup can't, then there is a gap between these two(your local and our expectation)

@huicunjun
Copy link
Contributor Author

Are you using the correct version of go lint? Different versions could result in different style details.

image

Thank ,You're right.

I started using the latest version of golangci 1.61.0.
Later, I changed it to the version specified in the project, and it was reduced to v1.20.0, which is the same as the project requirements, and the verification passed.

@mrproliu
Copy link
Contributor

Your plugin test failure, please fix it.

CodePrometheus
CodePrometheus previously approved these changes Sep 20, 2024
test/plugins/scenarios/goframe/bin/startup.sh Outdated Show resolved Hide resolved
@wu-sheng
Copy link
Member

@mrproliu Please recheck.

mrproliu
mrproliu previously approved these changes Sep 20, 2024
Copy link
Contributor

@mrproliu mrproliu left a comment

Choose a reason for hiding this comment

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

Thanks! Once the CI passed, I think it could be merged.

@huicunjun
Copy link
Contributor Author

Thanks! Once the CI passed, I think it could be merged.

Can you help me take a look this ci tags check problem?

@huicunjun
Copy link
Contributor Author

Thanks! Once the CI passed, I think it could be merged.

Why does local execution of run.sh goframe --debug pass the test

But Testcases: is 0 ,Log printing shows that the testing interface has been requested

image
image

@mrproliu
Copy link
Contributor

That's just a shell bug that shows the wrong scenario count, you can ignore it.

@mrproliu mrproliu merged commit bc3ac92 into apache:main Sep 20, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants