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

ci: Replace python go proto script with ci target #27675

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

phlax
Copy link
Member

@phlax phlax commented May 29, 2023

This replaces the current python script to build the go protos with a bash ci target, and removes the git logic which has been shifted downstream

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@phlax phlax marked this pull request as draft May 29, 2023 07:58
@phlax phlax force-pushed the bazel-go-protos branch 7 times, most recently from 4cc036a to fcc3df2 Compare May 29, 2023 12:25
Copy link
Contributor

@alecholmez alecholmez left a comment

Choose a reason for hiding this comment

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

This was migrated to go-control-plane 👍

@phlax phlax force-pushed the bazel-go-protos branch 5 times, most recently from 7043021 to c0affda Compare May 29, 2023 16:51
@phlax
Copy link
Member Author

phlax commented May 30, 2023

not sure why ci is failing. I tested it locally and got it working, then pushed and it failed due to RBE, so reproed locally with RBE and fixed, but its still failing 8/

@phlax phlax force-pushed the bazel-go-protos branch 4 times, most recently from 2b44f4c to d26d0af Compare May 30, 2023 12:17
@phlax
Copy link
Member Author

phlax commented May 30, 2023

debugging CI fail further locally - afaict the issue is because the api is being built previously, and as a result its using the cache and not downloading the go files - it may be related to bazelbuild/bazel#13882 - i think it probably passed previously due to building the go files first

adding --remote_download_outputs=all seems to resolve and didnt seem to have a huge perf penaltly when testing locally

@phlax phlax changed the title [WIP] ci: Remove python go proto script ci: Remove python go proto script May 30, 2023
@phlax phlax marked this pull request as ready for review May 30, 2023 12:54
@phlax phlax assigned phlax and alecholmez and unassigned phlax May 30, 2023
@phlax
Copy link
Member Author

phlax commented May 30, 2023

this needs envoyproxy/go-control-plane#701 in place before it can land

im also wondering whether we should replace the python script with a stub to raise an error with a "Please use ci/do_ci.sh api.go instead" message

cc @jamesmulcahy

@phlax
Copy link
Member Author

phlax commented May 30, 2023

/wait

@phlax
Copy link
Member Author

phlax commented Jun 1, 2023

updated the script to message and exit(1)

also added an ENVOY_API_ONLY env var to optionally skip building the go protobufs

@phlax
Copy link
Member Author

phlax commented Jun 1, 2023

@alecholmez there is a bit of a chicken/egg situation here - we kinda need to simultaneously update downstream

i guess its possible we could do it in steps, ill raise a PR to update and use the new target now

Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Member Author

phlax commented Jun 1, 2023

there is a bit of a chicken/egg situation here

apologies for confusion - thinking further - downstream has its own implementation so this just needs to land first

@alecholmez
Copy link
Contributor

All good! I'll standby for merging in go-control-plane

Copy link
Contributor

@alecholmez alecholmez left a comment

Choose a reason for hiding this comment

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

Looks good to me!

]) > 0
def main():
# TODO(phlax): remove this 12/23
sys.stderr.write("This script has been removed, please use `ci/do_ci.sh api.go` instead\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just delete this script? Less cruft n the tools folder

Copy link
Member Author

Choose a reason for hiding this comment

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

im a little concerned that others may be using it so would rather add this ~deprecation/removal notice for now

@phlax phlax changed the title ci: Remove python go proto script ci: Replace python go proto script with ci target Jun 1, 2023
Copy link
Member

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

@phlax phlax merged commit d29dcb1 into envoyproxy:main Jun 2, 2023
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
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