Fix crash on startup #312
Fix crash on startup #312
Conversation
| func (tn *TransformNetwork) GetServiceFiles() (string, string) { | ||
| p := tn.ProtocolsFile | ||
| if p == "" { | ||
| p = "/etc/protocols" |
There was a problem hiding this comment.
by the way, I used the previous default, as you can see here: https://github.com/netobserv/flowlogs-pipeline/pull/304/files#diff-803cd3d555b315c0ffdf0ac4905fd947085c6edf178137e7734e7af8a1218214L55 it was previously using these paths.
But maybe this isn't super safe to write in /etc/protocols? maybe rather use /tmp/flp-protocols and /tmp/flp-services instead? @KalmanMeth @ronensc @mariomac ?
There was a problem hiding this comment.
never mind, I thought it was a file that we would write into, I didn't know it was a standardized thingy
"failed to initialize pipeline opening "": open : no such file or directory" Previous PR removed the default file names for service mapping
jotak
force-pushed
the
default-filenames
branch
from
4826700
to
0afae26
Compare
| return nil, fmt.Errorf("opening protocols file %q: %w", pFilename, err) | ||
| } | ||
| defer protos.Close() | ||
| services, err := os.Open(jsonNetworkTransform.ServicesFile) | ||
| services, err := os.Open(sFilename) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("opening %q: %w", jsonNetworkTransform.ServicesFile, err) | ||
| return nil, fmt.Errorf("opening services file %q: %w", sFilename, err) |
There was a problem hiding this comment.
+1 for improving the error message
There was a problem hiding this comment.
I was able to find the error message in the e2e test of the PR that introduced the bug. But, despite that, the test passes.
https://github.com/netobserv/flowlogs-pipeline/actions/runs/3082582328/jobs/4982452400
There was a problem hiding this comment.
yeah seems like there's some improvement to bring to the e2e test too
| require.NoError(t, err) | ||
| require.NotNil(t, cfg) | ||
| mainPipeline, err := pipeline.NewPipeline(&cfg) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
This test is failing here with:
can't access kubenetes. Tried using config from: config parameter, KUBECONFIG env, homedir and InClusterConfig. Got: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined
I wonder if using os.Setenv is enough...
There was a problem hiding this comment.
Another option would be to remove the add_kubernetes rules from the test config.
There was a problem hiding this comment.
yeah, Ronen's suggestion is much cleaner for a unit test
There was a problem hiding this comment.
I'm rather mocking the kube client loading
There was a problem hiding this comment.
(else, kube client initialization would fail without a real kube cluster available)
| @@ -47,7 +47,13 @@ const ( | |||
| typeService = "Service" | |||
| ) | |||
|
|
|||
| type KubeDataInterface interface { | |||
There was a problem hiding this comment.
nitpick: this interface can be private.
There was a problem hiding this comment.
done (I just had to move the mock into kubernetes package :) )
jotak
force-pushed
the
default-filenames
branch
from
8988c41
to
f1e8240
Compare
"failed to initialize pipeline opening "": open : no such file or directory"
Previous PR removed the default file names for service mapping