-
Notifications
You must be signed in to change notification settings - Fork 0
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
[GAC-50] [flightsql-odbc] Fail the workflow if build fails #14
base: alinaliBQ/GAC-39/update-arrow-ver-flightsql-odbc
Are you sure you want to change the base?
[GAC-50] [flightsql-odbc] Fail the workflow if build fails #14
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.
LGTM! (Please do not merge the PR, we can just leave it open)
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format?
or
In the case of PARQUET issues on JIRA the title also supports:
See also: |
9494c65
to
2fd7a18
Compare
f491591
to
927f222
Compare
5c03e97
to
e6ba2c8
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.
Good PR. I see that the workflow fails properly when the build fails. I just have 2 minor comments. Will take another look when the test code in main.cc
is removed.
927f222
to
06ce06b
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.
LGTM!
Rationale for this change
CMAKE build results were being swallowed after the cd ..
Now the result is captured and returned later
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?