-
Notifications
You must be signed in to change notification settings - Fork 23
Fix gateway controller deploy target #674
Fix gateway controller deploy target #674
Conversation
a768f5b
to
01fbf11
Compare
@maleck13 @laurafitzgerald @sergioifg94 The e2e tests are currently not testing what we think they are testing (policy controller is always using the published quay.io main image). If one of you has a minute to review, would be good to get this merged ASAP. |
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.
@mikenairn left a couple of questions inline. none are blockers for merging this.
I've completed the verification steps and the expected images are running in cluster.
@@ -0,0 +1,10 @@ | |||
# Local deployment overlay. |
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 could be a nit:
And not essential for this change.
For a number of things I'd suggest some naming updates.
For example the
- config/deploy is referring to gateway-controller
- config/policy-controller is referring to policy-controller
Also - mgc-controller-manager is used for deployment/pod naming for gateway-controller
- mgc-policy-controller is used for deployment/pod naming for policy-controller
technically mgc-policy-controller could be expanded to multicluster-gateway-controller-policy-controller which could cause some confusion. maybe the prefix isn't appropriate anymore.
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 could be a nit: And not essential for this change.
For a number of things I'd suggest some naming updates. For example the
* config/deploy is referring to gateway-controller * config/policy-controller is referring to policy-controller
If it was intending to stay this way I'd agree, something like config/policy-controller
and config/gateway-controller
would make more sense, but moving the policy config was a first step and will hopefully soon be moved out of this repo entirely.
The policy-controller will eventually be it's own repo, and the current contents of config/policy-controller
should end up being the contents of config
in that repo and removed entirely from here.
Also * mgc-controller-manager is used for deployment/pod naming for gateway-controller * mgc-policy-controller is used for deployment/pod naming for policy-controller
I agree there were some naming inconsistencies brought in during the last couple of PRs (#644, #648, #667) that need resolved. Not really my intention to try and fix all of that here though.
technically mgc-policy-controller could be expanded to multicluster-gateway-controller-policy-controller which could cause some confusion. maybe the prefix isn't appropriate anymore.
I'm not sure what this should end up being, but it should have a prefix i think that is added to all resources created as part of this deployment. Currently that is mgc.
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 could be a nit: And not essential for this change.
For a number of things I'd suggest some naming updates. For example the* config/deploy is referring to gateway-controller * config/policy-controller is referring to policy-controller
If it was intending to stay this way I'd agree, something like
config/policy-controller
andconfig/gateway-controller
would make more sense, but moving the policy config was a first step and will hopefully soon be moved out of this repo entirely.The policy-controller will eventually be it's own repo, and the current contents of
config/policy-controller
should end up being the contents ofconfig
in that repo and removed entirely from here.
cool. no need for this change then.
Also * mgc-controller-manager is used for deployment/pod naming for gateway-controller * mgc-policy-controller is used for deployment/pod naming for policy-controller
I agree there were some naming inconsistencies brought in during the last couple of PRs (#644, #648, #667) that need resolved. Not really my intention to try and fix all of that here though.
yeah that's fair. I think it would be good to address them. And it's not essential to fix them here. We could create a follow on issue about it?
technically mgc-policy-controller could be expanded to multicluster-gateway-controller-policy-controller which could cause some confusion. maybe the prefix isn't appropriate anymore.
I'm not sure what this should end up being, but it should have a prefix i think that is added to all resources created as part of this deployment. Currently that is mgc.
01fbf11
to
6ce9ea6
Compare
`make deploy-gateway-controller` deploys both the gateway and policy controller but was only updating the image for the gateway controller to the locally built version. This chnage updates the make targets to ensure that both the gateway and policy controller images are updated when testing the combined gateway/policy controller installation (e2e tests).
6ce9ea6
to
667b02c
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maleck13, mikenairn The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
make deploy-gateway-controller
deploys both the gateway and policy controller but was only updating the image for the gateway controller to the locally built version. This change updates the make targets to ensure that both the gateway and policy controller images are updated when testing the combined gateway/policy controller installation (e2e tests).When deploying the controller using a local image loaded into kind, we also need to change the imagePullPolicy to Never or it will never start.
Verification
Check the gateway and policy deployments are both using the local image