-
Notifications
You must be signed in to change notification settings - Fork 385
Conversation
These passed on the last run when only running the controller tests (https://app.circleci.com/pipelines/github/hashicorp/consul-helm/1773/workflows/b4a96a44-1670-417d-ac62-1d80255f497f/jobs/4440) |
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.
🦊
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.
I see the tests pass locally! Just added a comment checking whether we need to continue overriding global.image or not.
@@ -78,7 +77,7 @@ func TestControllerNamespaces(t *testing.T) { | |||
"connectInject.enabled": "true", | |||
|
|||
// todo: remove when Helm chart updates to 1.8.4 | |||
"global.image": "hashicorp/consul-enterprise:1.8.4-ent", | |||
"global.image": "hashicorp/consul-enterprise:1.9.0-ent-beta1", |
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.
In test/acceptance/framework/config.go, I see setIfNotEmpty(helmValues, "global.image", entImage)
, which will set the image to "hashicorp/consul-enterprise:1.8.4-ent" since the Chart version is 1.8.4. It seems like we would manually override this whenever our Chart version does not match the enterprise image we want to use. So in this case do we specifically want to be on a higher version of consul enterprise to make these tests pass, or could we remove this line?
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.
Oops, yeah that comment is incorrect. The controller tests use the service-intentions CRD which requires 1.9.0.
So really that comment should say:
// todo: remove when Helm chart updates to 1.9.0
I'll push that change.
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.
Sweet, thanks!
These pass now with the latest changes.