Skip to content

ci/format: Bazelify check_format script#29397

Merged
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:format-bazel
Sep 5, 2023
Merged

ci/format: Bazelify check_format script#29397
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:format-bazel

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Sep 3, 2023

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 changed the title ci/format: Bazelify check_format script [WIP] ci/format: Bazelify check_format script Sep 3, 2023
@phlax phlax marked this pull request as draft September 3, 2023 15:44
@phlax phlax force-pushed the format-bazel branch 2 times, most recently from 1040368 to c6fccc0 Compare September 3, 2023 16:50
@repokitteh-read-only repokitteh-read-only Bot added the deps Approval required for changes to Envoy's external dependencies label Sep 3, 2023
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @RyanTheOptimist

🐱

Caused by: #29397 was synchronize by phlax.

see: more, trace.

@phlax phlax force-pushed the format-bazel branch 12 times, most recently from 28c4cf1 to 5a7c2e7 Compare September 4, 2023 07:05
@phlax phlax changed the title [WIP] ci/format: Bazelify check_format script ci/format: Bazelify check_format script Sep 4, 2023
@phlax phlax changed the title ci/format: Bazelify check_format script ci/format: Bazelify check_format script Sep 4, 2023
@phlax phlax marked this pull request as ready for review September 4, 2023 07:29
@phlax phlax force-pushed the format-bazel branch 2 times, most recently from 3e16e45 to c595c3b Compare September 4, 2023 07:39
@phlax phlax force-pushed the format-bazel branch 2 times, most recently from e3c19e6 to 0589928 Compare September 4, 2023 07:59
Comment thread bazel/repository_locations.bzl Outdated
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

cc @moderation this was pinned to this version on the basis that this is what we currently use an my initial testing suggested that a version change might create a flood of linting changes (not 100% but thought it better to separate that update - #29405 - unfortunately github is borked atm so hard to test)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ci is ~working again and confirmed that upgrading brings a load of change

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

phlax commented Sep 5, 2023

/retest

Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

With this change will people still run ./ci/run_envoy_docker.sh './ci/do_ci.sh check_format' in order to fix the format for Envoy and ./tools/check_format.sh fix to fix the format for Envoy Mobile? If not, we presumably need to announce this change to developers?

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Sep 5, 2023

With this change will people still run ./ci/run_envoy_docker.sh './ci/do_ci.sh check_format' in order to fix the format for Envoy

yes

.... and ./tools/check_format.sh fix to fix the format for Envoy Mobile?

this command has been updated to ...

$ bazel run //tools/code_format:check_format -- fix ...

If not, we presumably need to announce this change to developers?

im happy to shout out on the dev channel when it lands

@repokitteh-read-only repokitteh-read-only Bot removed the deps Approval required for changes to Envoy's external dependencies label Sep 5, 2023
@RyanTheOptimist
Copy link
Copy Markdown
Contributor

With this change will people still run ./ci/run_envoy_docker.sh './ci/do_ci.sh check_format' in order to fix the format for Envoy

yes

.... and ./tools/check_format.sh fix to fix the format for Envoy Mobile?

this command has been updated to ...

$ bazel run //tools/code_format:check_format -- fix ...

If not, we presumably need to announce this change to developers?

im happy to shout out on the dev channel when it lands

Thanks, that'd be great!

@phlax phlax merged commit 6b6ae44 into envoyproxy:main Sep 5, 2023
./library/common/extensions ./test/java ./test/kotlin ./test/objective-c
./test/swift ./experimental/swift)

ENVOY_BAZEL_PREFIX=@envoy ../tools/code_format/check_format.py "${FORMAT_ARGS[@]}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Removing this breaks the script when not run from CI.

joaopgrassi added a commit to dynatrace-oss-contrib/envoy that referenced this pull request Sep 13, 2023
Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
phlax added a commit to phlax/envoy that referenced this pull request Sep 19, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>
phlax added a commit to phlax/envoy that referenced this pull request Sep 19, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>
phlax added a commit that referenced this pull request Sep 20, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>
phlax added a commit that referenced this pull request Sep 21, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>
mum4k added a commit to envoyproxy/nighthawk that referenced this pull request Oct 27, 2023
Turns out our file format checkers stopped working some time ago (likely when the `--path` argument was added here: envoyproxy/envoy#29397). Once the path was set, the formatting tools executed, but they were checking format of Envoy files instead of Nighthawk files.

Most formatting changes were done automatically except:
- addition of missing Python docstrings.
- replacement of references to `google::protobuf` by references to the `Envoy::Protobuf` namespace.

Fixes #815 

Signed-off-by: Jakub Sobon <mumak@google.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