-
Notifications
You must be signed in to change notification settings - Fork 88
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
Uses viper for reading configurations for controller/watcher #1409
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1409 +/- ##
==========================================
- Coverage 61.65% 61.55% -0.10%
==========================================
Files 136 136
Lines 10205 10299 +94
==========================================
+ Hits 6292 6340 +48
- Misses 3408 3455 +47
+ Partials 505 504 -1
☔ View full report in Codecov by Sentry. |
awesome thanks, i can live with that tradeoff which is much more efficient than having to reload all config everytime and bring a performance boost... if people are asking we can add a /reload or something to PaC (may be with network ACL) |
748e357
to
e2550c2
Compare
@chmouel it would be nice if I can get access to cancel github workflow when not required to run 🙃 Also we export functions for Operator for validation/defaulting they have a map from TektonConfig CR, they will have to convert into viper and pass to PAC func |
@@ -71,6 +71,7 @@ jobs: | |||
|
|||
# restart controller | |||
kubectl -n pipelines-as-code delete pod -l app.kubernetes.io/name=controller |
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.
update..
kubectl patch configmap -n pipelines-as-code -p "{\"data\":{\"bitbucket-cloud-check-source-ip\": \"false\"}}" \
--type merge pipelines-as-code
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.
TODO:
- cleanup
- cli changes
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.
what are the cli changes that would be needed?
@piyush-garg do you think that would break the operator integration? |
now we mount configurations configmap for both componenet and use viper to read the configurations. NOTE: after updating the configmap it takes a little time to reflect in pod well that's because kubelet sync them on certain interval so if you don't see immediate change, then wait :P
reflecting change in configuration in pod take time so we can add a wait or restart pods, we already do controller so now watcher too.
e2550c2
to
463efd9
Compare
closing for now, incomplete need some work |
(Still need some work to make it work)
here are the changes which would be required if we go ahead with this
why?
WORST WORST CASE: 60-90s (although that would be rare)
A hacky way to to fix is.. updating pac pod with an annotation, which triggers kubelet to do it immediately.
Submitter Checklist
make test
before submitting a PR (ie: with pre-commit, no need to waste CPU cycle on CI. (or even better install pre-commit and dopre-commit install
in the root of this repo).make lint
before submitting a PR. The markdownlint error can get usually fixed by runningmake fix-markdownlint
(make sure it's installed first)