-
Notifications
You must be signed in to change notification settings - Fork 102
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
Expose service configuration #112
Conversation
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
@arschles @tomkerkhove can you PTAL? Just finished |
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 @khaosdoctor. Can you add documentation to the reference?
Will do! |
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.
@khaosdoctor one more thing - can you add a test for this too?
Signed-off-by: Lucas Santos <lhs.santoss@gmail.com>
Commited the requested changes, but haven't done the test yet because I'm having some issues running Go in my PC, also I think we'll need to run |
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.
@khaosdoctor will this mean we need to make changes to kedacore/charts#137 as well?
Most likely yes, since the CRD will change (we may need to think in an automated way to do this) |
@khaosdoctor in that case, do you think we need to bump the version of the CRD? |
This might be a good idea, since this is a breaking change I propose a major version increase |
would it, though? if you're just adding a new field to the spec, it won't break old |
Agreed, we can keep the version and change the release :) |
@khaosdoctor this looks great. I want to get the 0.1.0 release out before we merge this though. stand by :) |
Closing it as it's gonna be superseeded by #206 |
Adding exposed configuration, for now the only configs that can be changed are the service exposed port and type because labels are defined by the add-on.
Checklist
Fixes #107