chaostoolkit-kubernetes icon indicating copy to clipboard operation
chaostoolkit-kubernetes copied to clipboard

Terminate pod return value

Open hendrikKahl opened this issue 4 years ago • 6 comments

Hi,

this PR would add a return value to the terminate_pods action. I found it quite helpful to get this kind of information persisted in the journal.json document for future reference and further analysis. It would also address #60 .

Since it seems only the return value (or exception) of an action will be stored, I would suggest to store the already available pod names in an array and return it.

Tests have been enhanced to include checks of the return value.

Looking forward to get some thoughts on this suggestion.

Cheers, Hendrik

hendrikKahl avatar Apr 22 '20 14:04 hendrikKahl

Hello @hendrikKahl, this has been so long and I haven't paid attention enough to this PR. I apologise.

I see you had made a merge from master, I usually prefer a rebase. Would you be able to join me here so we can discuss if you still have bandwith to look at this PR again?

Lawouach avatar Sep 23 '20 10:09 Lawouach

Hi @Lawouach no worries, thanks for looking into the PR :)

I'd be happy to discuss the next steps. Please let me know, what you have in mind.

hendrikKahl avatar Sep 28 '20 13:09 hendrikKahl

Thanks for tolerating my slowness here :)

The best, if you could, start from master and only make a commit for your changes? Either you still have this branch somewhere and try a rebase, or feel free to create a new PR altogether. Whatever is simpler .

Lawouach avatar Sep 28 '20 13:09 Lawouach

Please excuse my lack of knowledge and understanding here, but I might need a hint or two to proceed.

I went back to my fork, branched off before the merge commit, rebased to incorporate latest changes from upstream and then cherry-picked my changes into this local branch.

While the commit tree look ok now, rebasing seems to alter the authors section of each commit while re-applying them on my fork. Not sure, if this would be kept, when I open a new PR...

The other option I could try, would be to create a branch in my fork without the merge commit and without any upstream changes. Again I could cherry-pick my change and reference this branch in a separate PR to bring in the single commit.

Any suggestion would be highly appreciated. Thanks for your understanding.

hendrikKahl avatar Sep 29 '20 07:09 hendrikKahl

Hi @Lawouach,

with a bit of time I managed to get a clean commit tree, but in a different branch. So I created #104 and would close this PR. Hope that's ok with you.

Cheers, Hendrik

hendrikKahl avatar Oct 07 '20 08:10 hendrikKahl

Codecov Report

Merging #89 (73e1899) into master (9b56dff) will increase coverage by 0.07%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   85.61%   85.69%   +0.07%     
==========================================
  Files          10       10              
  Lines         598      601       +3     
==========================================
+ Hits          512      515       +3     
  Misses         86       86              
Impacted Files Coverage Δ
chaosk8s/pod/actions.py 95.91% <100.00%> (+0.12%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9b56dff...73e1899. Read the comment docs.

codecov-io avatar Dec 01 '20 04:12 codecov-io