Docker-Compose: Avoid setting hostname to ''

[glensc]

The hostname is literally set to '':

Before:

# docker-compose exec smtp hostname
''

After:

# docker-compose exec smtp hostname
c531c19841de

[aminvakil]

I think there was a reason this has been added in 6c92774 which was in #1116.

Investigating further...

[aminvakil]

I just tested your change with docker-compose 2.3.2 and after this, snuba container keeps restarting without any log.

[glensc]

@aminvakil in any case, the used syntax is incorrect. it's already well known (to me at least) that docker compose doesn't like extra quoting. it's not shell script. for example if you place a variable with spaces to .env YOU MAU NOT QUOTE THE VALUE, or you get quotes in env value itself.

anyway, after my fix, if env is missing the valuer is set to empty (rather apostrhophes), and then default hostname will be set, which is generated name. as seen in example I provided

[glensc]

I fail to see how "smtp" container "hostname" is related to "snuba" container :D

If you insist on quoting, then this might work (not tested):

  smtp:
    <<: *restart_policy
    image: tianon/exim4
    hostname: "${SENTRY_MAIL_HOST:-}"

but that's just yaml syntax, shouldn't affect the variables expansion.

[aminvakil]

I just tested your change with docker-compose 2.3.2 and after this, snuba container keeps restarting without any log.

Ok, reverting this PR change results in sentry-self-hosted-snuba-subscription-consumer-transactions-1 container restarting over and over again.

[aminvakil]

I fail to see how "smtp" container "hostname" is related to "snuba" container :D

If you insist on quoting, then this might work (not tested):

  smtp:
    <<: *restart_policy
    image: tianon/exim4
    hostname: "${SENTRY_MAIL_HOST:-}"

but that's just yaml syntax, shouldn't affect the variables expansion.

Yeah, it was a mistake, it's not related. But I still want to understand why it has been added at the first place.

[aminvakil]

Right now our CI is failing (#1341) I will close and open this PR to retrigger 2.0.1 at least to see if this leads to any problem.

[BYK]

GitHub did something to how they use Docker across matrix jobs and I think this is causing issues. We may want to scope the project name with the job name in docker-compose setups to avoid the issues. The failure on 1.29.2 seems related to that.

[aminvakil]

@glensc As de81299 has been merged, can you please rebase your branch to retrigger the CI?

It should pass now.

[glensc]

@aminvakil, you can do that yourself, really:

[aminvakil]

@glensc Do you see "maintainer" here?

[aminvakil]

LGTM.

Closing and reopening to trigger the CI.

[aminvakil]

And CI is green. 🎉
(At least the part that must be)

[chadwhitacre]

Sadly needs another rebase once #1375 lands.

[chadwhitacre]

I landed #1375. Can you rebase again @glensc? 🙏

[glensc]

Why do you need me to do the rebases? Makes only useless ping-pongs!

[chadwhitacre]

/gcbrun

[chadwhitacre]

@glensc Do you see "member" here?

Oh wait ... 😬 🙈 😅

[glensc]

Build flakiness due to multiple versions in same docker instance still not resolved:

• https://github.com/getsentry/self-hosted/runs/5540753414?check_suite_focus=true

 Container self-hosted-0-nginx-1  Recreated
  Error response from daemon: Renaming a container with the same name as its current name
  ++ teardown ERR 45

The fix didn't apparently work:

• de81299

[chadwhitacre]

Build flakiness due to multiple versions in same docker instance still not resolved:

#1294 ftr

The fix didn't apparently work

de81299 was for #1341, not #1294. We have several open CI issues. 😞

[chadwhitacre]

/gcbrun

[chadwhitacre]

@glensc I'll force-merge if we hit CI flakiness again.

[aminvakil]

We have hit #1171 now :)
Let's merge this IMO.

[glensc]

I thought this was to solve container name collisions:

COMPOSE_PROJECT_NAME: self-hosted-${{ strategy.job-index }}

[glensc]

hey. CI green. merge fast before someone requests rebase! :D

[chadwhitacre]

Close one! Thanks @glensc! 😁