Skip to content
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

Consistently quote values in the docker-compose configurations #781

Merged
merged 2 commits into from
Jun 24, 2020

Conversation

ankon
Copy link
Contributor

@ankon ankon commented Jun 24, 2020

This commit aligns the quoting style to always use single quotes rather than an mix
of double quotes, single quotes, and no quotes.

The "no quotes" also introduced a rather ugly other problem: at least podman-compose locally
gets confused by environment variables like KAFKA_BROKER_ID: 0, and basically drops these
out, probably because '0' is a falsy value in YAML. This leads to Kafka starting up without
a broker id, and thereby it generates one for itself -- which the tests then are very unhappy about:

 FAIL  src/cluster/__tests__/brokerPool.spec.js
  ● Cluster > BrokerPool › #refreshMetadata › updates the list of brokers

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Array [
    -   "0",
        "1",
    +   "1001",
        "2",
      ]

      178 |       expect(brokerPool.brokers).toEqual({})
      179 |       await brokerPool.refreshMetadata([topicName])
    > 180 |       expect(Object.keys(brokerPool.brokers).sort()).toEqual(['0', '1', '2'])
          |                                                      ^
      181 |       expect(Object.values(brokerPool.brokers)).toEqual(
      182 |         expect.arrayContaining([expect.any(Broker), expect.any(Broker), expect.any(Broker)])
      183 |       )

      at Object.<anonymous> (src/cluster/__tests__/brokerPool.spec.js:180:54)

As environment variables should always be strings anyways: Fix that by just always quoting all environment
variables, to reduce the mental load of having all of YAMLs arcane rules in ones head.

This commit aligns the quoting style to always use single quotes rather than an mix
of double quotes, single quotes, and no quotes.

The "no quotes" also introduced a rather ugly other problem: at least podman-compose locally
gets confused by environment variables like `KAFKA_BROKER_ID: 0`, and basically drops these
out, probably because '0' is a falsy value in YAML. This leads to Kafka starting up without
a broker id, and thereby it generates one for itself -- which the tests then are very unhappy about:

~~~~
 FAIL  src/cluster/__tests__/brokerPool.spec.js
  ● Cluster > BrokerPool › #refreshMetadata › updates the list of brokers

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Array [
    -   "0",
        "1",
    +   "1001",
        "2",
      ]

      178 |       expect(brokerPool.brokers).toEqual({})
      179 |       await brokerPool.refreshMetadata([topicName])
    > 180 |       expect(Object.keys(brokerPool.brokers).sort()).toEqual(['0', '1', '2'])
          |                                                      ^
      181 |       expect(Object.values(brokerPool.brokers)).toEqual(
      182 |         expect.arrayContaining([expect.any(Broker), expect.any(Broker), expect.any(Broker)])
      183 |       )

      at Object.<anonymous> (src/cluster/__tests__/brokerPool.spec.js:180:54)
~~~~

As environment variables should always be strings anyways: Fix that by just always quoting all environment
variables, to reduce the mental load of having all of YAMLs arcane rules in ones head.
ankon added a commit to Collaborne/kafkajs that referenced this pull request Jun 24, 2020
This applies the changes from tulios#781 to the new docker-compose.2_4.yml file to simplify
merging of these PRs.
@Nevon
Copy link
Collaborator

Nevon commented Jun 24, 2020

Fun, only slightly related fact, I once worked on a product that had different features enabled in different countries. To manage this configuration, we were using YAML and simple lists of country codes. Something like this:

newFeatureEnabled:
  - se
  - fi
  - gb

At some point we added support for Norway:

newFeatureEnabled:
  - se
  - fi
  - gb
  - no

But to our great surprise, it didn't work in Norway. In fact, I seem to recall us having some issue after introducing Norway into this list. Turns out, our YAML parser helpfully converted the string no to the boolean value false, because that makes a lot of sense.

I don't love YAML.

@Nevon Nevon merged commit cff2cfc into tulios:master Jun 24, 2020
ankon added a commit to Collaborne/kafkajs that referenced this pull request Jun 25, 2020
This applies the changes from tulios#781 to the new docker-compose.2_4.yml file to simplify
merging of these PRs.
ankon added a commit to Collaborne/kafkajs that referenced this pull request Jun 25, 2020
This applies the changes from tulios#781 to the new docker-compose.2_4.yml file to simplify
merging of these PRs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants