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
Configurable task role #2
Configurable task role #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is awesome.
providers/aws/fargate/pod.go
Outdated
@@ -33,6 +33,9 @@ const ( | |||
|
|||
// Reason used for task state changes. | |||
taskGenericReason = "Initiated by user" | |||
|
|||
// Annotation to configure the task role |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please end comment lines with a '.'
providers/aws/fargate/pod.go
Outdated
@@ -51,6 +54,7 @@ type Pod struct { | |||
taskCPU int64 | |||
taskMemory int64 | |||
containers map[string]*container | |||
taskRoleArn *string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please move this to between taskArn and taskStatus? So that all ARNs are together. Also, any reason for the type to be *string? Let's just store it as string, like the others.
providers/aws/fargate/cluster.go
Outdated
@@ -223,6 +223,7 @@ func (c *Cluster) loadPodState() error { | |||
pod.taskArn = *task.TaskArn | |||
pod.taskStatus = *task.LastStatus | |||
pod.taskRefreshTime = time.Now() | |||
pod.taskRoleArn = taskDef.TaskRoleArn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this new line to between taskArn and taskStatus, to follow the same order they were declared in the struct. See my other comment.
providers/aws/fargate/pod.go
Outdated
if val, ok := pod.Annotations[taskRoleAnnotation]; ok { | ||
taskDef.TaskRoleArn = aws.String(val) | ||
} | ||
fgPod.taskRoleArn = taskDef.TaskRoleArn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please move this inside the if block? fdPod.taskRoleArn is only set if there is an annotation (ok == true), so shouldn't it be also inside the if block? Simply fdPod.taskRoleArn = val
Configurable task role via `iam.amazonaws.com/role`, which is also used by kube2iam.
93b2458
to
9f91146
Compare
Thank you @ofiliz, feedback addressed. |
Configurable task role via
iam.amazonaws.com/role
, which is also usedby kube2iam.