Save execution namespace in system metadata #568
Conversation
Signed-off-by: Katrina Rogan <[email protected]>
katrogan
requested review from
wild-endeavor,
EngHabu,
kumare3,
pmahindrakar-oss,
hamersaw and
eapolinario
as code owners
go.mod
Outdated
| @@ -13,7 +13,7 @@ require ( | |||
| github.com/cloudevents/sdk-go/v2 v2.8.0 | |||
| github.com/coreos/go-oidc v2.2.1+incompatible | |||
| github.com/evanphx/json-patch v4.12.0+incompatible | |||
| github.com/flyteorg/flyteidl v1.5.5 | |||
| github.com/flyteorg/flyteidl v1.5.7-0.20230522164738-0484cf8e75cc | |||
There was a problem hiding this comment.
will rebase before merging
Codecov Report
@@ Coverage Diff @@
## master #568 +/- ##
==========================================
+ Coverage 58.46% 60.00% +1.54%
==========================================
Files 168 168
Lines 16154 13218 -2936
==========================================
- Hits 9444 7932 -1512
+ Misses 5871 4447 -1424
Partials 839 839
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -287,6 +287,7 @@ func TestCreateExecution(t *testing.T) { | |||
| assert.Equal(t, rawOutput, spec.RawOutputDataConfig.OutputLocationPrefix) | |||
| assert.True(t, proto.Equal(spec.ClusterAssignment, &clusterAssignment)) | |||
| assert.Equal(t, "launch_plan", input.LaunchEntity) | |||
| assert.Equal(t, spec.GetMetadata().GetSystemMetadata().Namespace, "project-domain") | |||
There was a problem hiding this comment.
can we add one test with namespace mapping which equals just project
There was a problem hiding this comment.
The code is already testing that we're exercising namespace mapping config but added another test
| @@ -650,6 +650,7 @@ func (m *ExecutionManager) launchSingleTaskExecution( | |||
| UserInputsURI: userInputsURI, | |||
| SecurityContext: executionConfig.SecurityContext, | |||
| LaunchEntity: taskIdentifier.ResourceType, | |||
| Namespace: namespace, | |||
There was a problem hiding this comment.
Can we also make a backward compat change for older executions which dont have this namespace metadata set that it returns the project-domain which is widely used namespace
There was a problem hiding this comment.
I think we should respect the config if we're going to assume values rather than use the default mapping
There was a problem hiding this comment.
Yeah that works. wanted to make sure we have some data that we have valid data when we call the GET api even though the model wont have saved this when creating the execution
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
|
PTAL @pmahindrakar-oss |
Signed-off-by: Katrina Rogan <[email protected]>
TL;DR
Save execution namespace in system metadata
Type
Are all requirements met?
Complete description
N/A see summary
Tracking Issue
fixes flyteorg/flyte#3705
Follow-up issue
NA