Properly catch errors in ldflag-gathering and fail the build #5539
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The dubious-ownership issue in CI appears to be happening here too, and might be why some builds have "unknown" as their commit SHAs. Building this locally properly fails to build binaries when it cannot be gathered, and includes the data when it can: ``` VERSION: CLI feature version: 1.7.0 Release version: v1.2.7-prerelease2-dirty Build commit: 2023-12-22T20:30:21+00:00-d4d205fe3 ```
Groxx
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
taylanisikdemir
approved these changes
Jan 5, 2024
|
hmmm apparently there are still gaps in that. gonna hunt for a higher-level place to do it. |
|
much better: gonna clean up a bit. though hmmmm. it should have failed before. checking further... |
|
dangit.
so we should probably just add shellcheck. |
|
dunno what this implies: and going to ignore it for now. |
Groxx
commented
Jan 6, 2024
| # that's fine, just override it if it looks like we're in buildkite. | ||
| # elsewhere it's probably best to not touch this, and the path is likely wrong. | ||
| if [ -n "$BUILDKITE" ]; then | ||
| git config --global --add safe.directory /cadence |
There was a problem hiding this comment.
this is now done in the buildkite dockerfile, which appears to catch everything much more simply.
Groxx
commented
Jan 6, 2024
taylanisikdemir
approved these changes
Jan 7, 2024
Groxx
commented
Jan 8, 2024
| @@ -130,11 +130,11 @@ wait_for_postgres() { | |||
| wait_for_es() { | |||
| server=`echo $ES_SEEDS | awk -F ',' '{print $1}'` | |||
| URL="http://$server:$ES_PORT" | |||
| curl -s $URL 2>&1 > /dev/null | |||
| curl -s $URL > /dev/null 2>&1 | |||
There was a problem hiding this comment.
TIL. I've gotta adjust my muscle memory apparently.
Groxx
added a commit
to Groxx/cadence
that referenced
this pull request
Jan 9, 2024
uber#5539 / commit a8921b2 unfortunately has a bad bit of copypasta that changed the `cadence-server` build to make the CLI. This restores that makefile target to what it should be. tbh I'm not quite sure how this got past CI. Maybe we don't actually run the binary anywhere as part of an E2E test? It is fairly quick to notice at least though, since it causes problems quickly.
Groxx
added a commit
that referenced
this pull request
Jan 9, 2024
#5539 / commit a8921b2 unfortunately has a bad bit of copypasta that changed the `cadence-server` build to make the CLI. This restores that makefile target to what it should be. tbh I'm not quite sure how this got past CI. Maybe we don't actually run the binary anywhere as part of an E2E test? It is fairly quick to notice at least though, since it causes problems quickly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The dubious-ownership issue in CI appears to be happening here too,
and might be why some builds have "unknown" as their commit SHAs.
Building this change locally properly fails to build binaries when
it cannot be gathered, and includes the data when it can:
Previously there were basically three issues at play:
safe.directorything, now applied everywhere rather than just one target(AFAICT) be completely undetectable elsewhere in Make.
Since fixing the "always include the version, fail build if it does not work" issue
involved discovering and fixing all three, they're all in this PR.
We should seriously consider adding shellcheck to our CI pipeline.