-
Notifications
You must be signed in to change notification settings - Fork 3k
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
ci/docker: Add !operator dir into Dockerfile.dockerignore #14069
Conversation
00f07d2
to
80e7f06
Compare
@@ -28,4 +28,5 @@ | |||
!/pkg | |||
!/plugins/cilium-cni | |||
!/proxylib | |||
!/operator |
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.
this is not ideal in my opinion, as cilium agent now is having depepency with operator 😓. Maybe it's side effect of having centralized AWS config 1b1134c.
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.
another approach is to do a little bit refactor (i.e. move related common code to pkg). Keen to hear your inputs 👍
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.
another approach is to do a little bit refactor (i.e. move related common code to pkg)
It looks like the operator/option
package in needed in order to access the configuration of the operator:
cilium/pkg/aws/endpoints/resolver.go
Lines 32 to 41 in 51342a3
func Resolver(service, region string) (aws.Endpoint, error) { | |
if ep := operatorOption.Config.EC2APIEndpoint; len(ep) > 0 && service == "ec2" { | |
log.Debugf("Using custom API endpoint %s for service %s in region %s", ep, service, region) | |
// See https://docs.aws.amazon.com/sdk-for-go/v2/api/aws/endpoints/#hdr-Using_Custom_Endpoints | |
return aws.Endpoint{ | |
URL: "https://" + ep, | |
}, nil | |
} | |
return defaultResolver.ResolveEndpoint(service, region) | |
} |
I don't have much context on this part of the codebase but to me it doesn't look trivial to refactor the code in order to get rid of that import as Resolver
is used as a callback in a couple of places:
➜ cilium git:(master) rg endpoints.Resolver pkg/
pkg/aws/ec2/ec2.go
73: cfg.EndpointResolver = aws.EndpointResolverFunc(endpoints.Resolver)
pkg/policy/groups/aws/aws.go
65: cfg.EndpointResolver = aws.EndpointResolverFunc(endpoints.Resolver)
/cc @tklauser which may have a bit more context on this
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.
this is not ideal in my opinion, as cilium agent now is having depepency with operator 😓. Maybe it's side effect of having centralized AWS config 1b1134c.
The agent shouldn't have a dependency on the operator (and currently doesn't AFAIK). However, some of the packages which are also consumed by the agent depend on the operator/option
package which, is fine as it only contains the operator options. This was the reason in the first place to split out that package.
I think we should be able to solve this issue by changing from !operator
to !operator/option
in Dockerfile.ignore
.
Full CI is not required in my opinion, otherwise we could have caught this in related PR. |
This commit is to exclude operator/option directory in Dockerfile.dockerignore file to fix below issue ``` ../pkg/aws/endpoints/resolver.go:18:2: cannot find package "." in: 12 9.053 /go/src/github.com/cilium/cilium/operator/option ``` Signed-off-by: Tam Mach <[email protected]>
80e7f06
to
c0905a9
Compare
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.
LGTM, thanks 🚀
Description
This commit is to add operator directory in Dockerfile.dockerignore file to fix below issue
Signed-off-by: Tam Mach [email protected]
Full log can be found here https://github.com/cilium/cilium/runs/1417805368
Testing
Testing was done locally only as I just realise that image job in github action is kicked off only for master and tag (maybe we don't want to override the image in docker registries)
make -C images cilium-image