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.

Sign up for GitHub

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

Jump to bottom

GKEStartPodOperatorAsync #347

Merged
merged 5 commits into from
May 26, 2022
Merged

GKEStartPodOperatorAsync #347

merged 5 commits into from
May 26, 2022

Conversation

raphaelauv
Copy link
Contributor

@raphaelauv raphaelauv commented May 13, 2022 •
edited
Loading

close : #177

@kaxil kaxil requested a review from dstandish May 13, 2022 17:36
kaxil
kaxil reviewed May 13, 2022
View reviewed changes
Copy link
Collaborator

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

Thank you very much for starting this PR ❤️ , do let us know if you would like any help getting it across the line

@codecov
Copy link

codecov bot commented May 16, 2022 •
edited
Loading

Codecov Report

Merging #347 (5a20973) into main (bcc2b29) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #347      +/-   ##
==========================================
- Coverage   96.78%   96.78%   -0.01%     
==========================================
  Files          56       56              
  Lines        2925     2924       -1     
==========================================
- Hits         2831     2830       -1     
  Misses         94       94
Impacted Files Coverage Δ
...nomer/providers/google/cloud/operators/bigquery.py 100.00% <ø> (ø)

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 bcc2b29...5a20973. Read the comment docs.

phanikumv
phanikumv reviewed May 16, 2022
View reviewed changes
astronomer/providers/google/cloud/operators/bigquery.py Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@raphaelauv raphaelauv marked this pull request as ready for review May 16, 2022 21:43
kaxil
kaxil reviewed May 17, 2022
View reviewed changes
setup.cfg Outdated Show resolved Hide resolved
kaxil
kaxil reviewed May 17, 2022
View reviewed changes
setup.cfg Outdated Show resolved Hide resolved
kaxil
kaxil reviewed May 17, 2022
View reviewed changes
Copy link
Collaborator

@kaxil kaxil left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you add an example DAG and tests similar to https://github.com/astronomer/astronomer-providers/blob/main/tests/cncf/kubernetes/operators/test_kubernetes_pod.py please.

Do let us know if you want to take it from here

@pankajastro
Copy link
Contributor

pankajastro commented May 17, 2022 •
edited
Loading

In the airflow repo, the sync version of this operator has file namekubernetes_engine.py what about keeping the same name here?

@pankajastro
Copy link
Contributor

pankajastro commented May 17, 2022 •
edited
Loading

Can you please add the relevant description for this PR?

@pankajastro pankajastro linked an issue May 17, 2022 that may be closed by this pull request
Implement GKEStartPodOperatorAsync #177
Closed
bharanidharan14
bharanidharan14 reviewed May 19, 2022
View reviewed changes
Copy link
Contributor

@bharanidharan14 bharanidharan14 left a comment

Choose a reason for hiding this comment

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

Looks fine to me can you address one more comment on the example DAG

@raphaelauv @phanikumv
apply suggestion
5a20973 
Co-authored-by: Phani Kumar <94376113+phanikumv@users.noreply.github.com>
@pankajastro
Copy link
Contributor

LGTM. Let's merge this PR, Example DAG we will address in separate PR.

phanikumv
phanikumv approved these changes May 26, 2022
View reviewed changes
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.

Congrats @raphaelauv on your first merged PR

@phanikumv phanikumv merged commit c8f12de into astronomer:main May 26, 2022
@pankajastro
Copy link
Contributor

Hi @raphaelauv How did you test it? I'm trying to run it but it is failing, tracking my investigation here #390

@raphaelauv raphaelauv deleted the feature/GKEStartPodOperatorAsync branch May 30, 2022 08:06
@phanikumv phanikumv added this to the 1.5.0 milestone Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phanikumv phanikumv phanikumv approved these changes

@pankajastro pankajastro pankajastro approved these changes

@dstandish dstandish Awaiting requested review from dstandish

@rajaths010494 rajaths010494 Awaiting requested review from rajaths010494

@pankajkoti pankajkoti Awaiting requested review from pankajkoti pankajkoti is a code owner

@kaxil kaxil Awaiting requested review from kaxil

@bharanidharan14 bharanidharan14 Awaiting requested review from bharanidharan14

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.

Implement GKEStartPodOperatorAsync
5 participants
@raphaelauv @pankajastro @kaxil @phanikumv @bharanidharan14