search

konpyutaika / nifikop

The NiFiKop NiFi Kubernetes operator makes it easy to run Apache NiFi on Kubernetes. Apache NiFI is a free, open-source solution that support powerful and scalable directed graphs of data routing, transformation, and system mediation logic.
https://konpyutaika.github.io/nifikop/
Apache License 2.0
131 stars 45 forks source link

Update logback.xml template for 2.0.0-M1 and make it optional #353

Open juldrixx opened 8 months ago

juldrixx commented 8 months ago
Q A
Bug fix? no
New feature? yes
API breaks? no
Deprecations? no
Related tickets partial #360
License Apache 2.0

What's in this PR?

Update of the logback.xml template, to match the one in 2.0.0-M1. And made it optional (by default disabled).

Why?

NiFi added new appender and updated some of them. And there is no reason to override the default logback.xml with our template when we don't change anything in it. I made a function to check if the template needs to be used, but right now it just return false because nothing seems to make use of the template.

Checklist

mh013370 commented 8 months ago

Was this logback.xml just copied from the 2.0.0-M1 baseline?

mh013370 commented 8 months ago

We should also update the nifi-cluster helm chart logback.xml since it routes all of the appender output to the Console (stdout). It's verbatim copied and it includes an additional Console appender that basically accepts all the other appender outputs. Without this, only the bootstrap log appears in stdout, making it impossible to see nifi logs in something like the Rancher UI.

juldrixx commented 8 months ago

Was this logback.xml just copied from the 2.0.0-M1 baseline?

Yes directly copied from the docker image.

juldrixx commented 8 months ago

We should also update the nifi-cluster helm chart logback.xml since it routes all of the appender output to the Console (stdout). It's verbatim copied and it includes an additional Console appender that basically accepts all the other appender outputs. Without this, only the bootstrap log appears in stdout, making it impossible to see nifi logs in something like the Rancher UI.

I made the changes, can you confirm them. I'm not sure.

mh013370 commented 8 months ago

We should also update the nifi-cluster helm chart logback.xml since it routes all of the appender output to the Console (stdout). It's verbatim copied and it includes an additional Console appender that basically accepts all the other appender outputs. Without this, only the bootstrap log appears in stdout, making it impossible to see nifi logs in something like the Rancher UI.

I made the changes, can you confirm them. I'm not sure.

Looks good to me

mh013370 commented 8 months ago

Do you think there's any risk in merging this for nifikop 1.x?

juldrixx commented 8 months ago

Do you think there's any risk in merging this for nifikop 1.x?

I discussed with @erdrix yesterday about the suppression of the file. And he said that we shouldn't. The objective to have it in the operator like the others files was that the operator will provide its version of the configuration. That could be identitical to the one in NiFi but could be different (better) like for example in case where we could have some use of the NiFi logs in the Operator. And if the user wants the one from NiFi, it should override it. So we should make sure to keep them up to date. What do you think?

mh013370 commented 8 months ago

Do you think there's any risk in merging this for nifikop 1.x?

I discussed with @erdrix yesterday about the suppression of the file. And he said that we shouldn't. The objective to have it in the operator like the others files was that the operator will provide its version of the configuration. That could be identitical to the one in NiFi but could be different (better) like for example in case where we could have some use of the NiFi logs in the Operator. And if the user wants the one from NiFi, it should override it. So we should make sure to keep them up to date. What do you think?

Do you mean that we should keep the templates in nifikop?

juldrixx commented 8 months ago

Do you think there's any risk in merging this for nifikop 1.x?

I discussed with @erdrix yesterday about the suppression of the file. And he said that we shouldn't. The objective to have it in the operator like the others files was that the operator will provide its version of the configuration. That could be identitical to the one in NiFi but could be different (better) like for example in case where we could have some use of the NiFi logs in the Operator. And if the user wants the one from NiFi, it should override it. So we should make sure to keep them up to date. What do you think?

Do you mean that we should keep the templates in nifikop?

Maybe yes, but we have to see them as the optimal configuration from the point of view of the operator.

mh013370 commented 8 months ago

Do you think there's any risk in merging this for nifikop 1.x?

I discussed with @erdrix yesterday about the suppression of the file. And he said that we shouldn't. The objective to have it in the operator like the others files was that the operator will provide its version of the configuration. That could be identitical to the one in NiFi but could be different (better) like for example in case where we could have some use of the NiFi logs in the Operator. And if the user wants the one from NiFi, it should override it. So we should make sure to keep them up to date. What do you think?

Do you mean that we should keep the templates in nifikop?

Maybe yes, but we have to see them as the optimal configuration from the point of view of the operator.

Hmm, okay. I think as long as we keep compatibility well documented (which versions of nifikop are compatible w/ which versions of NiFi), this is okay. I only proposed this as a way to simplify what the operator has to care about.