Submodule protobuf #41
Conversation
|
commit 38e5568 was triggered automatically by the workflow added in this PR. The workflow is triggered on push. It first updates submodules, copies new protofbuf artifacts into the sdk, commits, and pushes back to the branch the triggered the workflow. main is a protected branch so it will not accept pushes from this workflow, but I haven't yet added anything to block the workflow from being triggered on push to main. I think a consequence of this is that PR merges into main will trigger this workflow and the workflow will then fail. I don't believe this will impact the merge to main at this time, but it might become an issue in the future and will need to be addressed (if not addressed by this PR). I'm also not sure triggering this update on push is the best CI step for this event and am open to discussion if anyone has an opinion. |
| git add ./src/**/*.proto | ||
| git commit -m "Updated protobuf files" | ||
| - name: Push change | ||
| uses: ad-m/github-push-action@master |
There was a problem hiding this comment.
I also lean towards having actions create PRs instead of just pushing changes (unless they are pushing to a branch which is automated only). So using an action like https://github.com/marketplace/actions/create-pull-request.
There was a problem hiding this comment.
Unless of course the plan is to not manually change the files under these paths, then I think the push could be ok as a result of a manual commit to update the submodule.
There was a problem hiding this comment.
yeah that was the idea. main is a protected branch so this can only be triggered on other branches. So it works, but also just typing that out makes me feel worse about it. not a huge fan of part of our CI depending on future us knowing that the main branch needs to stay protected. having it create a PR instead of pushing is probably a good idea. something something human intervention. go ahead and tag this PR as "changes requested"
There was a problem hiding this comment.
If the goal is to ensure that any changes that occur upstream in api-interfaces will eventually make its way into this repo, then having this workflow run periodically and having it create an MR when the interfaces change seems like the ideal way to go.
could you elaborate on why you favor this option? |
| cp $SRC/$KIND/${KIND}_pb2_grpc.py $TGT/$KIND/${KIND}_pb2_grpc.py | ||
| cp $SRC/$KIND/${KIND}_pb2.py $TGT/$KIND/${KIND}_pb2.py | ||
| ############################# | ||
| # for backwards compatibility |
There was a problem hiding this comment.
I don't know how I feel about copying these files out of the submodule. I know people already rely on this directory structure, but I wonder if it would be better for us to have a clean, versioned break. Open to arguments either way, though.
There was a problem hiding this comment.
I'm inclined to agree. Since people can now pull these generated files out of the api-interfaces, I think it makes sense to only have this project be concerned about the generated files meant for python consumption.
Like Chia said, we'll want to cut a major version so we don't break existing consumers.
There was a problem hiding this comment.
it's been less than a month since the beta release so the timing for a major version is good too. especially considering the third party ts client is currently broken because of this dependency already. maybe we do this PR leaving the backwards compatibility stuff in it as a temporary fix to get people who are using the third party ts client back on their feet, and then follow that up with a new major version. this would give a little bit of breathing room to users depending on the current structure to upgrade without requiring that their stuff be broken and unavilable while they're in transition
There was a problem hiding this comment.
alternatively, choosing not to support backwards compatibility is a good forcing function for upgrading
Mainly since the |
| name: Update protobuf stubs | ||
|
|
||
| on: | ||
| push: |
There was a problem hiding this comment.
Probably only want this running on main (once it is known to work), and possibly with a path filter to just when api-interfaces is changed.
|
Kudos, SonarCloud Quality Gate passed!
|
No description provided.