-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Debug CI #1273
Debug CI #1273
Conversation
|
bb6a550
to
bc03d78
Compare
Restarting debug ... |
Actually, does it fail locally? 🤔 |
Passes locally. |
That sounds like nginx is up but the app server is down. Why does it fail so late though? That's not the first time we hit the server, is it? |
So maybe giving #1267 a little love helps here too? |
Hm, maybe something changed in Relay? 🧐 |
Trying to repro locally ... |
Based on #1251 (comment) I made an attempt to get our relay invocation to not spit out
#!/usr/bin/env zsh
dc="./docker-compose-1.28.0"
$dc version
$dc run \
--no-deps \
--volume "$(pwd)/relay/config.yml:/tmp/config.yml" \
relay --config /tmp credentials generate --stdout |
@chadwhitacre let's add a |
My hunch is that the file is malformed rather than missing. We shall see ... |
Hmm ... not seeing the cat? Guess we need that |
You know what, I bet it's because we mount things to |
install/relay-credentials.sh
Outdated
@@ -20,6 +21,10 @@ if [[ ! -f "$RELAY_CREDENTIALS_JSON" ]]; then | |||
> "$RELAY_CREDENTIALS_JSON" |
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.
Maybe we should add that -T
here right after $dcr
? Since the file is there but it is empty, makes me think this is a stdout
redirection related issue.
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.
Is there a reason 07c025c won't work?
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.
Because, read the giant comment I left there? 😁
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.
I read it but I couldn't make sense of it.
relay generate credentials
tries to read the config and the credentials even with the--stdout
and--overwrite
flags and then errors out when the credentials file exists but not valid JSON. We hit this case as we redirect output to the same config folder, creating an empty credentials file before relay runs.
So no shell redirect means no credentials file created before relay runs means no error parsing JSON?
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.
Hard for me to recollect the whole memory but let me try:
- We do have a config for Relay in place, either one the end-user created or the one we copied from the example file
- When you run
relay generate credentials
even with--overwrite
it tries to read both the config and the credentials (this happens only when you have the config in place) - Relay yells at us if it cannot read/parse credentials file and stops
- Even when we say "ignore that, overwrite or write the output to
stdout
" Relay still acts like a fussy toddler
Hrmm ... the file exists but is seemingly empty. 🤔 |
Okay so that only works with |
Okay, I think the only reasonable fix here is to run |
Okay that explains https://github.com/getsentry/self-hosted/runs/5029137872?check_suite_focus=true#step:4:163. I can use the tail call in 5f67f7b. |
Btw, |
Does it? Link? |
Dunno, thought you tried that too :D Btw setting |
Ah, you mean without |
Oh! Locally I already have the image cached. |
Boom! 💥 I removed the relay nightly image and I can repro locally:
💃 |
But! We can work around the "redirect creates the empty file which confuses relay" issue by redirecting to a tmp file and moving that into place. So we're good there. 👍 Last thing I want to check is Docker Compose 2 behavior ... |
Alright I'm outta time. I think with some fresh eyes tomorrow it shouldn't take too long to get a PR together. Thanks for your help @BYK! 😁 💃 🌮 👏 |
Oh wow do the different CI runs use the same docker cache? 🤔 |
Because I'm surprised that 1.28.0 is passing 🤔 |
Looks like we need the |
Wow! Glad it was sorted out at last! |
CI is failing on master. Why?
#1272