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/codeql: Fix build target acquisition #31722

Merged
merged 2 commits into from
Jan 9, 2024
Merged

Conversation

phlax
Copy link
Member

@phlax phlax commented Jan 9, 2024

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
Copy link
Member Author

phlax commented Jan 9, 2024

cc @mmorel-35

ravenblackx
ravenblackx previously approved these changes Jan 9, 2024
@@ -6,7 +6,8 @@ permissions:
on:
push:
paths:
- 'source/common/**'
- include/**
- source/common/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you either include the ' delimiters like it used to, or also remove them from branches-ignore below, for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

ill remove them from below - that is more consistent with the rest of the code base

ravenblackx
ravenblackx previously approved these changes Jan 9, 2024
Signed-off-by: Ryan Northey <[email protected]>
@phlax phlax merged commit f8941a2 into envoyproxy:main Jan 9, 2024
23 of 26 checks passed

- name: Get build targets
run: |
# TODO(phlax): Shift this to an action
compare_head () {
while IFS= read -r line; do
echo "CHECK: ${line}"
Copy link
Contributor

Choose a reason for hiding this comment

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

@phlax doesn't this line affect the output of the function to include CHECK: ... as well as the output of the bazel query? I am curious since I am now seeing failures from this action in this PR because of the CHECK line. (logs from a failure here)

Copy link
Member Author

@phlax phlax Jan 15, 2024

Choose a reason for hiding this comment

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

it certainly should (EDIT: should not) be here - altho im a bit confused as it does appear to be working elsewhere - so im thinking its not the cause of your problem - which afaict is more to do with an empty line being returned

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