-
Notifications
You must be signed in to change notification settings - Fork 3
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
Finish internal build pipeline reusable workflow #107
Conversation
f8d79db
to
5d80cde
Compare
# 2. Integration tests passed at least via one job above | ||
if: | | ||
success() || | ||
inputs.dbms_name == 'spark' || |
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.
@mikealfare suggested we make this an env var but this is impossible based on how the execution runtime works. I tried about 5 other options (see commit history) and very simply none worked. It's because GHA has limited execution flow options with needs
. This key word is required to maintain order of execution, but as soon as you add it, you start having trouble with other mechanisms for conditional execution. But then also reusable workflows like the Postgres testing one must be invoked at the job level another step level so I can't use a conditional check inside of a job. Basically, my hands are tied on exactly what I can do and this is a way I can keep things working for now.
When we get this merged and I try mixing in the 3p repo changes, I can attempt to rework things to be better here.
eb6564e
to
3e19770
Compare
3e19770
to
0ce9030
Compare
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.
Overall looks great. There is one blocker with the conditional logic on building.
echo "HATCH_ADAPTERS='${{ env.HATCH_ADAPTERS }}'" >> $GITHUB_ENV | ||
ADAPTERS="$(jq -r '.' <<< "${HATCH_ADAPTERS}")" | ||
echo "$ADAPTERS" | ||
DBMS_NAME="${{ inputs.dbms_name }}" | ||
if jq -e --arg dbms "$DBMS_NAME" '.[] | select(. == $dbms)' <<< ${ADAPTERS}; then |
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.
This feels overly complicated. If HATCH_ADAPTERS is just a space delimited list you can just check that the dbms is in the list:
if [[ " ${{ env.HATCH_ADAPTERS}} " =~ " ${{ inputs.dbms_name }} " ]]; then
echo "is_hatch_adapter=true" >> $GITHUB_OUTPUT
else
echo "is_hatch_adapter=false" >> $GITHUB_OUTPUT
fi
I'm also fine with leaving this for now and simplifying it at a future date. Not a blocker.
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.
Let me address in the upcoming round 2 which will add third party support. But geez okay this is news to me. For what it's worth, I went with a comment delimited list as the goal but I realize now it's more bash idiomatic to use the space delimited list. It's rare I get to up my bash fu these days -- thanks for the prompt! 🙇♀️
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.
" ${{ env.HATCH_ADAPTERS}} " =~ " ${{ inputs.dbms_name }} "
This is how you say inputs.dbms_name
is in env.HATCH_ADAPTERS
in GHA?
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.
None of my comments are blocking, just name nits. Only one you should def do is to update the input description. Approving now, pending successful testing!
with: | ||
python-version: ${{ env.PYTHON_TARGET_VERSION }} | ||
|
||
- name: "Setup `hatch`" |
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.
This action includes setting up python
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.
There's nothing blocking, but I do think it's worth updating the comment on line 33 and removing the extra setup of python.
resolves 38 in the epic
Description
Finish this up for 1p repos.
Checklist