Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Fix duplicate env vars in container #358

Merged
merged 6 commits into from
Jun 28, 2023
Merged

Conversation

hamersaw
Copy link
Contributor

@hamersaw hamersaw commented Jun 8, 2023

TL;DR

Currently, when building a k8s Pod we make duplicate calls to AddFlyteCustomizationsToContainer (here and here. This PR updates the BuildRawPod function to call a refactored BuildRawContainer to ensure the AddFlyteCustomizationsToContainer is only called once so that env vars are only injected a single time.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

^^^

Tracking Issue

fixes flyteorg/flyte#3740

Follow-up issue

NA

Signed-off-by: Daniel Rammer <daniel@union.ai>
…ting container

Signed-off-by: Daniel Rammer <daniel@union.ai>
Signed-off-by: Daniel Rammer <daniel@union.ai>
@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #358 (eb5818f) into master (2c393c0) will increase coverage by 0.00%.
The diff coverage is 37.50%.

❗ Current head eb5818f differs from pull request most recent head 7f84f40. Consider uploading reports for the commit 7f84f40 to get more accurate results

@@           Coverage Diff           @@
##           master     #358   +/-   ##
=======================================
  Coverage   62.87%   62.88%           
=======================================
  Files         153      153           
  Lines       12893    12893           
=======================================
+ Hits         8107     8108    +1     
  Misses       4172     4172           
+ Partials      614      613    -1     
Flag Coverage Δ
unittests 62.88% <37.50%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...tasks/pluginmachinery/flytek8s/container_helper.go 85.37% <33.33%> (ø)
go/tasks/pluginmachinery/flytek8s/pod_helper.go 73.66% <100.00%> (+0.64%) ⬆️

... and 1 file with indirect coverage changes

Signed-off-by: Daniel Rammer <daniel@union.ai>
pingsutw
pingsutw previously approved these changes Jun 26, 2023
Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

worth adding a couple of unit tests to confirm behavior?

Signed-off-by: Daniel Rammer <daniel@union.ai>
Signed-off-by: Daniel Rammer <daniel@union.ai>
@hamersaw hamersaw merged commit df62599 into master Jun 28, 2023
5 checks passed
@hamersaw hamersaw deleted the bug/duplicate-env-vars branch June 28, 2023 13:49
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* removed duplicate execution of AddFlyteCustomizationsToContainer

Signed-off-by: Daniel Rammer <daniel@union.ai>

* removed duplicate call to AddFlyteCustomizationsToContainer when creating container

Signed-off-by: Daniel Rammer <daniel@union.ai>

* removed dead code

Signed-off-by: Daniel Rammer <daniel@union.ai>

* added unit test for duplicate environment variables

Signed-off-by: Daniel Rammer <daniel@union.ai>

---------

Signed-off-by: Daniel Rammer <daniel@union.ai>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants