-
Notifications
You must be signed in to change notification settings - Fork 55
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.
Thanks @letzya! Overall looks good, but requires a few changes.
tyk-headless/Chart.yaml
Outdated
name: tyk-headless | ||
appVersion: "v3.2.1" |
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.
IIRC, we've decided to remove appVersion
altogether since it can't track all the components.
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 don't about it. where was this decision published? regardless atm it's "1.0" and it's wrong. continue in slack
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 vaguely remember that it was made during our planning session. Maybe @excieve can provide more input on it?
As mentioned on slack, I'm inclined to remove appVersion in the Chart.yaml for all charts.
@abhishiekgowtham please resolve the comment if you see fit :) and review from the top. Thanks! |
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
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
mongo: {}
in tyk-pro/values.yaml since it's confusing. If you set the mongoURL because of the few comment lines you don't see that there're{}
aftermongo
that needs to be deleted. Adding a single field with the default value sort this out