-
Notifications
You must be signed in to change notification settings - Fork 125
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
e2e github actions #6303
base: cloud-e2e-setup-services
Are you sure you want to change the base?
e2e github actions #6303
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Functionality looks good. Just looks like there's one spot where the code could be simpler.
run: | | ||
echo "matrix=$(jq -n '[ | ||
"web-admin" | select(env.FILTER_ADMIN == "true" or env.FILTER_COMMON == "true"), | ||
"web-local" | select(env.FILTER_LOCAL == "true" or env.FILTER_COMMON == "true") | ||
]' | jq -c)" >> "$GITHUB_OUTPUT" | ||
env: | ||
FILTER_ADMIN: ${{ steps.filter.outputs.admin }} | ||
FILTER_COMMON: ${{ steps.filter.outputs.common }} | ||
FILTER_LOCAL: ${{ steps.filter.outputs.local }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like these environment variables are adding an extra layer of indirection. Could we use filter.outputs
directly in the jq
command like: select(${{ steps.filter.outputs.admin }} == "true" or ${{ steps.filter.outputs.common }} == "true")
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further, can we treat the filter output like a typical boolean, not a string, like:
select(${{ steps.filter.outputs.admin }} or ${{ steps.filter.outputs.common }})
or even without the select
function:
if ${{ steps.filter.outputs.admin }} or ${{ steps.filter.outputs.common }} then "web-admin" else empty end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it doesn't have access to steps.filter.outputs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? This example seems to show it's possible.
No description provided.