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: Update Proto + ~~ORM Support~~ #1589

Merged
merged 7 commits into from
Jun 2, 2022
Merged

chore: Update Proto + ~~ORM Support~~ #1589

merged 7 commits into from
Jun 2, 2022

Conversation

alexanderbez
Copy link
Contributor

A re-attempt of #1455, but I've reached the capacity of my knowledge here. Maybe @robert-zaremba or @aaronc can chime in on how to get this to the finish line.

Currently, running make proto-gen yields:

make proto-gen
Generating Protobuf files
Generating gogo proto code
WARNING: field Pool.ScalingFactor is a repeated non-nullable native type, nullable=false has no effect
WARNING: field Pool.ScalingFactor is a repeated non-nullable native type, nullable=false has no effect
WARNING: field MsgStableSwapAdjustScalingFactors.ScalingFactors is a repeated non-nullable native type, nullable=false has no effect
Cleaning API directory
./scripts/protocgen2.sh: cd: line 14: can't cd to api: No such file or directory
make: *** [proto-gen] Error 2

It also produces an erroneous v7/ dir in the root.

@aaronc
Copy link

aaronc commented May 26, 2022

Seems like the api directory is missing

@alexanderbez
Copy link
Contributor Author

Seems like the api directory is missing

But it seems like all the contents in that directory are auto-generated?

@aaronc
Copy link

aaronc commented May 26, 2022

Doesn't hurt to try a mkdir -p

@robert-zaremba
Copy link
Contributor

If you don't use pulsar then you don't need to generate api content.

@robert-zaremba
Copy link
Contributor

Originally, I was thinking to not include the ./scripts/protocgen2.sh until you will like to use pulsar and ORM.

@alexanderbez
Copy link
Contributor Author

Originally, I was thinking to not include the ./scripts/protocgen2.sh until you will like to use pulsar and ORM.

@ValarDragon stated we want to introduce support for ORM, but idk what that looks like.

@ValarDragon
Copy link
Member

Should we use Pulsar? I didn't realize ORM and Pulsar came together, my bad then. Would like to not have to force a switch of everything to the API module, I was under the impression from the regen codebase that just ORM things went there, and the normal proto files were being compiled as they are today

@ValarDragon
Copy link
Member

Those warnings are present on main as well

@aaronc
Copy link

aaronc commented May 26, 2022

Should we use Pulsar? I didn't realize ORM and Pulsar came together, my bad then. Would like to not have to force a switch of everything to the API module, I was under the impression from the regen codebase that just ORM things went there, and the normal proto files were being compiled as they are today

That's right. Currently there's a hybrid approach

@alexanderbez
Copy link
Contributor Author

I think we should still merge this as it uses the new buf stuff (heh), but just remove the protogen2 script. WDYT @ValarDragon?

@alexanderbez alexanderbez changed the title chore: Update Proto + ORM Support chore: Update Proto + ~~ORM Support~~ Jun 2, 2022
@alexanderbez alexanderbez marked this pull request as ready for review June 2, 2022 16:27
@alexanderbez alexanderbez requested a review from a team June 2, 2022 16:27
proto/buf.md Outdated Show resolved Hide resolved
@mergify mergify bot merged commit f291e12 into main Jun 2, 2022
@mergify mergify bot deleted the bez/orm-proto-updates branch June 2, 2022 18:50
@mattverse mattverse mentioned this pull request Jun 6, 2022
3 tasks
@czarcas7ic czarcas7ic mentioned this pull request Aug 19, 2022
mattverse added a commit that referenced this pull request Sep 6, 2022
mattverse added a commit that referenced this pull request Sep 13, 2022
* Revert "chore: Update Proto + ~~ORM Support~~ (#1589)"

This reverts commit f291e12.

* Update scripts/protocgen.sh

Co-authored-by: Roman <[email protected]>

* Run buf mod update

* query gen

* Change pb

Co-authored-by: Roman <[email protected]>
mergify bot pushed a commit that referenced this pull request Sep 13, 2022
* Revert "chore: Update Proto + ~~ORM Support~~ (#1589)"

This reverts commit f291e12.

* Update scripts/protocgen.sh

Co-authored-by: Roman <[email protected]>

* Run buf mod update

* query gen

* Change pb

Co-authored-by: Roman <[email protected]>
(cherry picked from commit 79adab9)
p0mvn pushed a commit that referenced this pull request Sep 14, 2022
… (#2720)

* Revert "chore: Update Proto + ~~ORM Support~~ (#1589)" (#2614)

* Revert "chore: Update Proto + ~~ORM Support~~ (#1589)"

This reverts commit f291e12.

* Update scripts/protocgen.sh

Co-authored-by: Roman <[email protected]>

* Run buf mod update

* query gen

* Change pb

Co-authored-by: Roman <[email protected]>
(cherry picked from commit 79adab9)

* Fix proto check

Co-authored-by: Matt, Park <[email protected]>
Co-authored-by: mattverse <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants