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

Apply 'go mod tidy' after API sync #908

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

mmorel-35
Copy link
Contributor

@mmorel-35 mmorel-35 commented Mar 22, 2024

@phlax ,
I believe there is no go mod tidy applied after envoy synchronisation

@phlax
Copy link
Member

phlax commented Mar 22, 2024

might be a good idea to add it

the sync happens here

build_protos

@mmorel-35 mmorel-35 marked this pull request as draft March 22, 2024 16:26
@mmorel-35 mmorel-35 marked this pull request as ready for review March 22, 2024 16:59
@mmorel-35
Copy link
Contributor Author

I couldn't test this, in codespaces, I have :

$ ENVOY_SRC_DIR=../envoy ci/sync_envoy.sh
Building go protos ...
ENVOY_SRCDIR=/workspaces/envoy
ENVOY_BUILD_TARGET=//source/exe:envoy-static
ENVOY_BUILD_ARCH=x86_64
BUILD_DIR not set - defaulting to ~/.cache/envoy-bazel

Bazel is not installed on codespaces machines maybe it is related

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.

dont worry about testing it we can land and if there is an issue we can revert

the only question i have is what go version this is using - one way/another i think we want to make sure its pinned

ci/sync_envoy.sh Outdated Show resolved Hide resolved
@mmorel-35 mmorel-35 marked this pull request as draft March 22, 2024 17:14
@phlax
Copy link
Member

phlax commented Mar 22, 2024

trying to rem how i tested previously - maybe i copied the workflow to trigger on pr altho cant rem if that is allowed for prs to do

@mmorel-35
Copy link
Contributor Author

I installed bazellisk on codespace, it's working better now

@mmorel-35 mmorel-35 closed this Mar 22, 2024
@mmorel-35 mmorel-35 reopened this Mar 22, 2024
@mmorel-35 mmorel-35 marked this pull request as ready for review March 22, 2024 20:21
@mmorel-35 mmorel-35 requested a review from phlax March 22, 2024 20:24
Signed-off-by: Matthieu MOREL <[email protected]>
@mmorel-35
Copy link
Contributor Author

@phlax ,
Shall there be a make go-mod-tidy target or use the already existing make clean instead of directly execute a go mod tidy ?

@phlax
Copy link
Member

phlax commented Mar 26, 2024

not sure - im kinda surprised this repo uses make tbh

iiuc you would mostly want to run tidy after changing deps so probs a standalone target - but i have limited go experience, so happy to defer to others with more knowledge of go workflows

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

I think what you have here is sufficient, a new make target would be a bit verbose for just running go mod tidy and make clean isn't necessary, that seems more for a local dev environment where you may have old binaries etc lying around

@sunjayBhatia sunjayBhatia changed the title apply 'go mod tidy' Apply 'go mod tidy' after API sync Mar 26, 2024
@sunjayBhatia sunjayBhatia merged commit d6aa65a into envoyproxy:main Mar 26, 2024
5 checks passed
@mmorel-35 mmorel-35 deleted the go-mod-tidy branch March 26, 2024 14:24
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