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

[BUG] Fix setting of service_account from PodTemplate #4536

Merged
merged 6 commits into from
Dec 6, 2023

Conversation

pvditt
Copy link
Contributor

@pvditt pvditt commented Dec 6, 2023

Tracking issue

#3613

Why are the changes needed?

When manually setting service_account_name in pod template on a flytekit task, the service account gets overridden by default (empty string).

@task(
    pod_template=PodTemplate(
        pod_spec=V1PodSpec(
            service_account_name="test-service-account",
            containers=[],
        )
    )
)
def say_hello() -> str:
    return "Hello, World! Hello World !"

What changes were proposed in this pull request?

Don't overwrite a pod's service account if it's already set.

How was this patch tested?

Unit test added here
Manually tested

Setup process

Screenshots

Screenshot 2023-12-05 at 9 13 07 PM

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Dec 6, 2023
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2105f09) 58.88% compared to head (688405e) 58.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4536      +/-   ##
==========================================
+ Coverage   58.88%   58.92%   +0.04%     
==========================================
  Files         620      620              
  Lines       52431    52432       +1     
==========================================
+ Hits        30874    30896      +22     
+ Misses      19090    19071      -19     
+ Partials     2467     2465       -2     
Flag Coverage Δ
unittests 58.92% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Paul Dittamo <[email protected]>
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Dec 6, 2023
Signed-off-by: Paul Dittamo <[email protected]>
Signed-off-by: Paul Dittamo <[email protected]>
@pvditt pvditt requested a review from hamersaw December 6, 2023 05:08
Signed-off-by: Paul Dittamo <[email protected]>
@pvditt pvditt requested a review from hamersaw December 6, 2023 19:09
@hamersaw hamersaw merged commit 2553153 into master Dec 6, 2023
41 checks passed
@hamersaw hamersaw deleted the bug/pod_template_service_account branch December 6, 2023 20:05
Copy link

welcome bot commented Dec 6, 2023

Congrats on merging your first pull request! 🎉

pvditt added a commit that referenced this pull request Dec 13, 2023
* don't override service account from security context if already set

Signed-off-by: Paul Dittamo <[email protected]>

* update unit test

Signed-off-by: Paul Dittamo <[email protected]>

* cleanup

Signed-off-by: Paul Dittamo <[email protected]>

* typo

Signed-off-by: Paul Dittamo <[email protected]>

* clean up sytling

Signed-off-by: Paul Dittamo <[email protected]>

---------

Signed-off-by: Paul Dittamo <[email protected]>
Co-authored-by: Dan Rammer <[email protected]>
Signed-off-by: Paul Dittamo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants