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

Fix get_logs, pod cleanup and XCom push in GKEStartPodOperatorAsync #824

Merged
merged 3 commits into from Jan 4, 2023

Conversation

bharanidharan14
Copy link
Contributor

@bharanidharan14 bharanidharan14 commented Dec 22, 2022

  • Fix Issue: Support get_logs in GKEStartPodOperatorAsync #811 not showing pod's logs on completion if get_logs is True
    Currently in GKEStartPodOperatorAsync, even though the get_logs attribute is set to True it was not showing the pod's log as in sync operator as expected, now it is fixed extended support to conclude the get_logs attribute once the trigger callback function is executed fetching the container logs from the pod manager if get_logs attribute is set to True.

  • Fix Issue Improve GKEStartPodOperatorAsync #451 pod cleanup and XCom push
    Once the trigger callback function is executed after the successful event. Pushed pod name and namespace and result in XCom when do_xcom_push is set to True and cleaning of the pod was also handled.

  • Handle the GKEStartPodOperatorAsync status based on the terminal status. Status of GKEStartPodOperatorAsync should reflect pod status. #814
    Currently, GKEStartPodOperatorAsync (and GKEStartPodTrigger) succeeds whenever the pod's state is one of PodPhase.terminal_states, that is either Failed or Succeeded. So by adding condition in the trigger based on the terminal status and returning the failed even when the terminal status is in a failed state.

closes: #811 #451 #814

@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

Base: 98.53% // Head: 98.54% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (9348101) compared to base (d939f5b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #824      +/-   ##
==========================================
+ Coverage   98.53%   98.54%   +0.01%     
==========================================
  Files          88       88              
  Lines        4845     4881      +36     
==========================================
+ Hits         4774     4810      +36     
  Misses         71       71              
Impacted Files Coverage Δ
...oviders/cncf/kubernetes/triggers/wait_container.py 98.63% <100.00%> (ø)
...viders/google/cloud/operators/kubernetes_engine.py 100.00% <100.00%> (ø)
...oviders/google/cloud/triggers/kubernetes_engine.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bharanidharan14
Copy link
Contributor Author

@pankajastro @pankajkoti @rajaths010494 @phanikumv Need review on this PR

Copy link
Collaborator

@phanikumv phanikumv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please share logs for all the points you've covered in this implementation

Copy link
Collaborator

@phanikumv phanikumv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description isnt as per standards, and has typos. Please correct the same

@bharanidharan14
Copy link
Contributor Author

Screenshot 2022-12-27 at 4 16 22 PM

Screenshot 2022-12-27 at 4 17 07 PM

Copy link
Collaborator

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Have a small question inline.

@bharanidharan14 bharanidharan14 changed the title Fix issues in GKEStartPodOperatorAsync Fix get_logs, pod cleanup and XCom push in GKEStartPodOperatorAsync Jan 3, 2023
@bharanidharan14
Copy link
Contributor Author

PR description isnt as per standards, and has typos. Please correct the same

@phanikumv Fixed it. Can you review it

@phanikumv
Copy link
Collaborator

PR description isnt as per standards, and has typos. Please correct the same

@phanikumv Fixed it. Can you review it

Description should be in imperative tense. Please correct it.

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.

Support get_logs in GKEStartPodOperatorAsync
5 participants