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

go: generate and save go protobufs #8

Merged
merged 9 commits into from
Nov 27, 2019
Merged

go: generate and save go protobufs #8

merged 9 commits into from
Nov 27, 2019

Conversation

kyessenov
Copy link
Contributor

@kyessenov kyessenov commented Nov 26, 2019

Pointing to a repository without go code is not valid go code outside of bazel environment:

udpa/service/orca/v1/orca.pb.go:9:2: module github.com/cncf/udpa@latest (v0.0.0-20191004202315-015fc86d90f4) found, but does not contain package github.com/cncf/udpa/udpa/data/orca/v1

The proposal is to use this repo to host the golang generated UDPA protos.

Signed-off-by: Kuat Yessenov kuat@google.com

Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov
Copy link
Contributor Author

cc @htuch @nareddyt

nareddyt
nareddyt previously approved these changes Nov 26, 2019
@htuch
Copy link
Collaborator

htuch commented Nov 26, 2019

@kyessenov I'm hesitant about forming dependencies on Envoy specific projects from UDPA. Is there no way to add some Go stubs or something in this repository?

@htuch htuch self-assigned this Nov 26, 2019
@kyessenov
Copy link
Contributor Author

We can add them here, right next to protos. Is this something you want to commit to? It's hard to change paths once we publish the go generated code (the package prefix is very important in golang).
We would need to set-up CI as well (I'm not seeing any CI checks).

@htuch
Copy link
Collaborator

htuch commented Nov 26, 2019

@kyessenov I think we do need them here ideally (for the UDPA ones). Would you be able to do the plumbing? We also need to setup CI, @lizan do you know the best path for AZP first CI?

@kyessenov
Copy link
Contributor Author

We also need to add go module versioning I believe and go manifests (go.mod file).

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov
Copy link
Contributor Author

kyessenov commented Nov 26, 2019

Added a script to store generated files. Checked manually with:

bazel test ...
pushd go; go build ./...; popd

We probably need a check that the repo is clean after re-generating the files.

@kyessenov kyessenov changed the title go: remap udpa go generated code to go-control-plane go: generate and save go protobufs Nov 26, 2019
Copy link
Collaborator

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks. This seems like a good first step here.

go/udpa/data/orca/v1/orca_load_report.pb.go Show resolved Hide resolved
os.chmod(output_file, 0o644)


if __name__ == "__main__":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Who runs this? Should there be a README.md?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added bare-bones developer README file.
It should be run by whoever wants to change the API.
It should really be a makefile, but I don't want to get into the debate of makefiles vs bazel.

bazel/generate_go_protobuf.py Outdated Show resolved Hide resolved
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@@ -0,0 +1,9 @@
module github.com/cncf/udpa/go
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NB: go module declaration cannot be placed at the root since go does not like C++ files in test/

Copy link
Collaborator

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@htuch htuch merged commit 5f054cc into cncf:master Nov 27, 2019
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.

3 participants