search

bakdata / kpops

Deploy Kafka pipelines to Kubernetes
https://bakdata.github.io/kpops
MIT License
12 stars 1 forks source link

Fix config.yaml overriding environment variables #353

Closed sujuka99 closed 1 year ago

disrupted commented 1 year ago

Please rephrase the PR title and start with "Fix ..." so that it's easy to spot bugfixes 🙂

sujuka99 commented 1 year ago

I would consider this a bugfix for something that should have been the default behavior. Therefore I would also remove the breaking-change label

I am not sure about that, this has always been KPOps' behavior.

The behavior is consistent and likely expected for some if not most of KPOps' users, so I wouldn't necessarily call it a bug.

If you are certain that it shouldn't be labeled breaking, I will remove it. I think you have a better grasp at the userbase :grin:

disrupted commented 1 year ago

I would consider this a bugfix for something that should have been the default behavior. Therefore I would also remove the breaking-change label

I am not sure about that, this has always been KPOps' behavior.

The behavior is consistent and likely expected for some if not most of KPOps' users, so I wouldn't necessarily call it a bug.

If you are certain that it shouldn't be labeled breaking, I will remove it. I think you have a better grasp at the userbase 😁

I totally understand your concern. In this case I strongly believe that the expected way would be that environment variables take precedence, hence why I was so surprised myself that it's not the case. Otherwise we would also have to do a major release, which – in this case – is unnecessary imo.