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

core: gateway response change #623

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

Yaiba
Copy link
Contributor

@Yaiba Yaiba commented Apr 1, 2024

This update gateway client to be compatible with new kgw repsonse structure, which is compatible with swagger error response

This requires both kwil-cli and kgw update

This should be merged once after kwilteam/kgw#29 is merged

@Yaiba Yaiba force-pushed the core/kgw-resp-change branch from f73575f to e339636 Compare April 4, 2024 22:44
@Yaiba Yaiba changed the title [WIP]: core: gateway response change core: gateway response change Apr 4, 2024
@Yaiba Yaiba force-pushed the core/kgw-resp-change branch from e339636 to 8f2ce60 Compare April 4, 2024 22:48
@Yaiba Yaiba marked this pull request as ready for review April 4, 2024 22:48
}

// gatewayAuthGetResponse defines the response of GET request for /auth
type gatewayAuthGetResponse struct {
Result *types.GatewayAuthParameter `json:"result"`
Error string `json:"error"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All KGW error will be in gatewayErrRespose, this is no use.

Copy link
Member

@jchappelow jchappelow Apr 5, 2024

Choose a reason for hiding this comment

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

manually triggered kgw against kgw main test passed: https://github.com/kwilteam/kwil-db/actions/runs/8571353728/job/23491389542 (that doesn't include https://github.com/kwilteam/kgw/pull/29, so is that weird to pass?)

manually triggered kgw against https://github.com/kwilteam/kgw/pull/29 just failed: https://github.com/kwilteam/kwil-db/actions/runs/8571621385 Couldn't find the kgw commit sha? Retry by kgw branch name instead of commit: https://github.com/kwilteam/kwil-db/actions/runs/8571658702/job/23492412534 looks to be running

Copy link
Contributor Author

@Yaiba Yaiba Apr 5, 2024

Choose a reason for hiding this comment

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

For test on main, I guess not, because we don't really test the error for KGW APIs?

For test on kwilteam/kgw#29, not sure

Comment on lines +126 to +129
// gatewayErrResponse defines the response of gateway error.
type gatewayErrResponse struct {
// to be compatible with google.golang.org/genproto/googleapis/rpc/status.Status
Message string `json:"message"`
Copy link
Member

Choose a reason for hiding this comment

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

@jchappelow
Copy link
Member

https://github.com/kwilteam/kgw/pull/29 is merged. I think kgw repo should have a replace for the core module (a commit on main) so that it uses this when it's merged.

@jchappelow
Copy link
Member

Or kgw update require to a pseudoversion of core

@jchappelow
Copy link
Member

Hmm, the kgw manual test run against kgw main and kwil-db main (without this PR) passed. https://github.com/kwilteam/kwil-db/actions/runs/8571925077/job/23493296723
Is that expected without these payload changes.

Anyway, this PR LGTM.

@Yaiba
Copy link
Contributor Author

Yaiba commented Apr 5, 2024

kwilteam/kgw#29 is merged. I think kgw repo should have a replace for the core module (a commit on main) so that it uses this when it's merged.

You mean for testing? In the workflow, if the branch is not 'release-*', it'll use 'go work', so it will use the core moudle(commin on 'main') on kwil-db

@Yaiba
Copy link
Contributor Author

Yaiba commented Apr 5, 2024

Hmm, the kgw manual test run against kgw main and kwil-db main (without this PR) passed. https://github.com/kwilteam/kwil-db/actions/runs/8571925077/job/23493296723 Is that expected without these payload changes.

Anyway, this PR LGTM.

yeah that's what I mentioned on the meeting, I didn't create a test on this error response case

@jchappelow
Copy link
Member

kwilteam/kgw#29 is merged. I think kgw repo should have a replace for the core module (a commit on main) so that it uses this when it's merged.

You mean for testing? In the workflow, if the branch is not 'release-*', it'll use 'go work', so it will use the core moudle(commin on 'main') on kwil-db

No, just for kgw build. The go.mod in the kgw repo does not have a replace or anything that would have it use latest kwil-db. We have to update it for every change to core that we want kgw to use.

@Yaiba
Copy link
Contributor Author

Yaiba commented Apr 5, 2024

kwilteam/kgw#29 is merged. I think kgw repo should have a replace for the core module (a commit on main) so that it uses this when it's merged.

You mean for testing? In the workflow, if the branch is not 'release-*', it'll use 'go work', so it will use the core moudle(commin on 'main') on kwil-db

No, just for kgw build. The go.mod in the kgw repo does not have a replace or anything that would have it use latest kwil-db. We have to update it for every change to core that we want kgw to use.

For local dev I also use 'go work', so it's not a problem? I don't think I get you

@jchappelow
Copy link
Member

Ah, sorry sorry, I see in .github/workflows/kgw-test-reuse.yaml: go work use . $kdbDir/core

@jchappelow jchappelow force-pushed the core/kgw-resp-change branch from 8f2ce60 to a17e15b Compare April 5, 2024 15:58
@Yaiba
Copy link
Contributor Author

Yaiba commented Apr 5, 2024

Ah, sorry sorry, I see in .github/workflows/kgw-test-reuse.yaml: go work use . $kdbDir/core

yeah, I think that will serve the purpose

@jchappelow jchappelow force-pushed the core/kgw-resp-change branch from a17e15b to 1645da1 Compare April 5, 2024 16:29
@jchappelow jchappelow merged commit 465af33 into kwilteam:main Apr 5, 2024
1 check passed
@jchappelow jchappelow added this to the v0.8.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants