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

tools: allow generate_go_protobuf.py to skip syncing with go-control-… #16287

Merged
merged 4 commits into from
May 4, 2021

Conversation

jamesmulcahy
Copy link
Contributor

Commit Message:

Currently, it also syncs them with go_control_plane, but it can be
useful for local development work to be able to just generate the
code

Additional Description:
Risk Level: Low
Testing: I've manually invoked the scripted and tested out the code. The existing behavior is retained as the default.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
[Optional Runtime guard:] n/a
[Optional Fixes #Issue] n/a
[Optional Deprecated:] n/a
[Optional API Considerations:] n/a

…plane

Currently, it also syncs them with go_control_plane, but it can be
useful for local development work to be able to just generate the
code

Signed-off-by: James Mulcahy <[email protected]>
@phlax phlax self-assigned this May 3, 2021
@phlax
Copy link
Member

phlax commented May 3, 2021

thanks @jamesmulcahy this looks good...

@@ -121,16 +121,24 @@ def updated(repo):


if __name__ == "__main__":
parser = argparse.ArgumentParser(description='Generate Go protobuf files and sync with go-control-plane')
parser.add_argument('--skip_sync', action='store_true')
Copy link
Member

@phlax phlax May 3, 2021

Choose a reason for hiding this comment

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

how about reversing this - ie --sync and store_false

hmm thats not going to work but we could keep store_true and update where its called in ci

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that works. I've made the change locally. Would you prefer a commit on top, or re-writing the original commit. I'm not 100% sure what the policy is here

Copy link
Member

@phlax phlax May 3, 2021

Choose a reason for hiding this comment

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

once there are reviewers, its preferred not to rebase/force - sometimes personally i find it necessary, but best avoided

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack. Extra commit pushed! 👍

@@ -142,3 +140,5 @@ def updated(repo):
new_sha = changes[0]
write_revision_info(repo, new_sha)
publish_go_protobufs(repo, new_sha)
else:
Copy link
Member

Choose a reason for hiding this comment

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

nit: bail early - ie if not args.sync: print + return

then you can dedent the following code

Copy link
Member

Choose a reason for hiding this comment

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

actually sys.exit i guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, extra commit pushed with those changes.

Signed-off-by: James Mulcahy <[email protected]>
phlax
phlax previously approved these changes May 3, 2021
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

lgtm - thanks @jamesmulcahy

cc @htuch for final review

@jamesmulcahy
Copy link
Contributor Author

@phlax FYI. The python formatting ci check failed, so I had to push an extra commit to fix a line wrapping issue. That led to your approval being dropped.

Copy link
Member

@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.

LGTM, thanks!

@htuch htuch merged commit fcb415e into envoyproxy:main May 4, 2021
gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 5, 2021
envoyproxy#16287)

Currently, it also syncs them with go_control_plane, but it can be
useful for local development work to be able to just generate the
code

Risk Level: Low
Testing: I've manually invoked the scripted and tested out the code. The existing behavior is retained as the default.

Signed-off-by: James Mulcahy <[email protected]>
Signed-off-by: Gokul Nair <[email protected]>
gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021
envoyproxy#16287)

Currently, it also syncs them with go_control_plane, but it can be
useful for local development work to be able to just generate the
code

Risk Level: Low
Testing: I've manually invoked the scripted and tested out the code. The existing behavior is retained as the default.

Signed-off-by: James Mulcahy <[email protected]>
Signed-off-by: Gokul Nair <[email protected]>
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