-
Notifications
You must be signed in to change notification settings - Fork 132
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
Override idls module to submodule to simplify idl update flow #1358
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@@ -114,3 +114,5 @@ retract ( | |||
v0.3.0 // Published accidentally | |||
v0.2.0 // Published accidentally | |||
) | |||
|
|||
replace github.com/uber/cadence-idl => ./idls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaces do not work for anyone importing the client, so this only works for local developers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while I like the idea and there might be something we can do to streamline this, I'm pretty confident this can't work the way we want it to. it'll just cause even-stranger issues when we make changes and fail to update one or the other, and they're less likely to be caught in CI since they'll only affect external users (due to replace).
What changed?
Switched go mod for cadence-idls to submodule so we need to only update submodule when we want to updates idls.
Why?
Eliminate a confusion of separate version of idls used for thrift and proto and separate version used.
How did you test it?
make build/make test
Potential risks