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: sort SDK backend api, upgrade dependencies and fix inbound misnomer #150

Merged
merged 3 commits into from
Dec 14, 2022

Conversation

kleyow
Copy link
Contributor

@kleyow kleyow commented Dec 14, 2022

No description provided.

@axolo-co
Copy link

axolo-co bot commented Dec 14, 2022

kleyow commented:
o - o)

Copy link
Member

0_0

Copy link
Member

😄

Copy link
Member

related to this PR

Copy link
Member

@kleyow #149 <-- check my comment

Copy link
Member

you have renamed the API but not the file

@axolo-co
Copy link

axolo-co bot commented Dec 14, 2022

kleyow commented:
I know. Github UI didn't intelligently pick up the file rename so there was no file diff and it would be impossible to review that.

I'll do the file/folder rename in my sorting PR since that is a non functional change PR.

@mdebarros

Copy link
Member

can you not create a snapshot release to check that?

@axolo-co
Copy link

axolo-co bot commented Dec 14, 2022

kleyow commented:
Then we can take a look at what exactly was desynced in the PR's I raised ~10 hours ago.

@kleyow kleyow changed the title chore: sort SDK backend api and upgrade dependencies chore: sort SDK backend api, upgrade dependencies and fix inbound misnomer Dec 14, 2022
@axolo-co
Copy link

axolo-co bot commented Dec 14, 2022

kleyow commented:
Explain? I think I get what you're saying.

Though, since the backend openapi is in the TTKs for solely in the functional tests for bulk. Creating a snapshot of api-snippets won't really help.
Mmm...basically the sdk has no tests that would run tests through the backend openapi so any desyncing problems would likely go unnoticed until someone implemented a core connector referencing the desynced backend api.

I also need to ask @vijayg10 how we can load spec files from api-snippets so we can use that as our source of truth.

mdebarros
mdebarros previously approved these changes Dec 14, 2022
Copy link
Member

@mdebarros mdebarros left a comment

Choose a reason for hiding this comment

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

+1

@axolo-co
Copy link

axolo-co bot commented Dec 14, 2022

vijayg10 commented:
@kleyow There is no built-in functionality to pull the remote spec, we should manage that using some init scripts before starting the service in container.

@mdebarros mdebarros self-requested a review December 14, 2022 14:08
@mdebarros mdebarros dismissed their stale review December 14, 2022 14:08

incorrectly approved

@axolo-co
Copy link

axolo-co bot commented Dec 14, 2022

vijayg10 commented:
Guys, the naming is a bit confusing. I think we should call "DFSP backend API" and "SDK backend API" in the conversations.

Copy link
Member

manually updated to the api definitions for the TTK Backend

Copy link
Member

Mmm...basically the sdk has no tests that would run tests through the backend openapi so any desyncing problems would likely go unnoticed until someone implemented a core connector referencing the desynced backend api.
☝️ I guess you are right….

Since the functional tests are running against the updated backend api definition

Copy link
Member

ok approved the wrong PR

@axolo-co
Copy link

axolo-co bot commented Dec 14, 2022

kleyow commented:
It's pure confusion...
I'll be naming it the SDK Backend API here and throughout.

@axolo-co
Copy link

axolo-co bot commented Dec 14, 2022

kleyow commented:
The bulk functional tests are actually running against a old backend openapi, just that the newly added bulk paths/components/etc don't break anything outside of that bulk scope 😅

Copy link
Member

Approved PR #149

Copy link
Member

@mdebarros mdebarros left a comment

Choose a reason for hiding this comment

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

  • 1

@axolo-co
Copy link

axolo-co bot commented Dec 14, 2022

kleyow commented:
Uhhh...sorry for the PR confusion. I wanted the "sort" PR's merged in first. That was the first step I did in the "syncing". The sort PR's will help eliminate diff so we can actually see the desyncing.

Copy link
Member

oh right

@kleyow kleyow merged commit 3d7c380 into master Dec 14, 2022
@kleyow kleyow deleted the chore/sort branch December 14, 2022 14:22
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