migrating influxdb e2e to go #3451
Conversation
Ritikaa96
force-pushed
the
influxdb-migration-to-golang
branch
2 times, most recently
from
817dff1 to
7a3e4f3
Compare
|
Hi , @JorTurFer @tomkerkhove @v-shenoy PTAL ! |
|
/run-e2e influx* |
|
Could you implement the activation too? I mean, as part of this PR |
sure, @JorTurFer but shouldn't the activation feature be added to the scaler first? Let me know what you think.. i'll start with the scaler meanwhile. |
|
Yes, you should add it to the scaler and also to the e2e. If you prefer to do in another split PR it's okey too |
you found one then. i'm on it! :D |
|
@JorTurFer Hi, i would make the change in influxdb_test.go in the pr for activation. you can go ahead with the merge.
|
@JorTurFer Hi, i would make the change in influxdb_test.go in the pr for activationValue. you can go ahead with the merge. |
tests/helper/helper.go
Outdated
| pods, err := kc.CoreV1().Pods(namespace).List(context.TODO(), | ||
| metav1.ListOptions{LabelSelector: label}) | ||
| if err != nil { | ||
| assert.NoErrorf(t, err, "no pod in the list - %s", err) | ||
| } | ||
| var podLogRequest *rest.Request | ||
| for _, v := range pods.Items { | ||
| podLogRequest = kc.CoreV1().Pods(namespace).GetLogs(v.Name, &corev1.PodLogOptions{}) | ||
| } |
There was a problem hiding this comment.
Are we assuming here only one pod matches the label (considering we overwrite the podLogRequest)?
There was a problem hiding this comment.
Are we assuming here only one pod matches the label (considering we overwrite the podLogRequest)?
hi @v-shenoy , yes , the function was made as per influxdb scaler & then transferred to helper so might have missed this detail. what do you suggest in this case ?
There was a problem hiding this comment.
Maybe move all the below lines into the loop, and we return a list of logs (although we don't have much use for it right now, but the function will be more general at least).
There was a problem hiding this comment.
that's what i am working on right now. or we can just list out name of pods because not many tests are trying to access the logs but a function for list of pods would be much more useful
What Do you think?
There was a problem hiding this comment.
Could you elaborate what you mean by list out name of pods?
There was a problem hiding this comment.
Hi @v-shenoy , generalised the function for logs in latest code ,as for the last comment : If the function were to return only a list of podnames then the scaler test would try finding whatever information regarding the pod using the particular pod name. It depends on scaler test demands if more tests demand pod details and not logs then we can keep it till the podList is given.
For time being i made the function generalised , thanks for the suggestion!
Signed-off-by: Ritikaa96 <ritika@india.nec.com>
Ritikaa96
force-pushed
the
influxdb-migration-to-golang
branch
from
7a3e4f3 to
d93ee85
Compare
|
/run-e2e influx* |
|
@v-shenoy @JorTurFer PTAL |
|
Thanks a lot @Ritikaa96 ! |
sure @JorTurFer !! |
|
We plan to do a release next week, do you think that you have enough time? We can do it of not, no pressure 😊 |
Signed-off-by: Ritikaa96 ritika@india.nec.com
Checklist
Fixes #3237
Relates to #2737