Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Terminate pod return value #89

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hendrikKahl
Copy link
Contributor

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

@Lawouach
Copy link
Contributor

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?

@hendrikKahl
Copy link
Contributor Author

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.

@Lawouach
Copy link
Contributor

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 .

@hendrikKahl
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@codecov-io
Copy link

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%) ⬆️

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants