Skip to content

[common-go] Add file watcher #10020

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

Merged
merged 5 commits into from
May 18, 2022
Merged

[common-go] Add file watcher #10020

merged 5 commits into from
May 18, 2022

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented May 14, 2022

Description

Refactor watch of configuration files and reload and remove duplication

How to test

  • Change ws-daemon or ws-daemon configmap and check the reload message appears in the log
{"level":"info","message":"reloading file after change","path":"/config/config.json","serviceContext":{"service":"ws-daemon","version":"commit-5de52b5daf5c35fe22f46adcff90cbaca78b075b"},"severity":"INFO","time":"2022-05-16T05:23:37Z"}}

Release Notes

[common-go] Add file watcher
[registry-facade] Refactor watch of configuration file
[ws-daemon] Refactor watch of configuration file

@aledbf
Copy link
Member Author

aledbf commented May 16, 2022

/werft run

👍 started the job as gitpod-build-aledbf-watcher.14
(with .werft/ from main)

@aledbf aledbf force-pushed the aledbf/watcher branch 2 times, most recently from bac375b to 4fdbccd Compare May 16, 2022 05:28
@aledbf aledbf marked this pull request as ready for review May 16, 2022 05:29
@aledbf aledbf requested review from a team May 16, 2022 05:29
@github-actions github-actions bot added team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team labels May 16, 2022
@utam0k
Copy link
Contributor

utam0k commented May 16, 2022

I love this feature. Do you have a plan to apply it to other components (e.g. ws-manager)?

@aledbf
Copy link
Member Author

aledbf commented May 16, 2022

I love this feature. Do you have a plan to apply it to other components (e.g. ws-manager)?

Not really. The only two components that support partial configuration reload are ws-daemon and ws-registry. That said, if we add such a feature to any other component, like ws-manager, we can add the four lines required to detect changes.

Copy link
Contributor

@utam0k utam0k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works fine

Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/spf13/viper looks to solve this problem, perhaps we could simplify our usage with it.

Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change makes a ton of sense.
Left two comments.

Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a Kubernetes controller and watches the ConfigMap change (resourceVersion change) is also a way.

@aledbf
Copy link
Member Author

aledbf commented May 17, 2022

Having a Kubernetes controller and watches the ConfigMap change (resourceVersion change) is also a way.

That means we need to watch the configmap using client-go. I would prefer to not add that here.

@aledbf
Copy link
Member Author

aledbf commented May 17, 2022

https://github.com/spf13/viper looks to solve this problem, perhaps we could simplify our usage with it.

Almost the same code https://github.com/spf13/viper/blob/v1.11.0/viper.go#L440

I am ok to use that but right now we are not using viper (another new dependency)

Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left a couple of comments but those do not block this PR and can be done as follow-up. Adding hold in case you want to tackle them in this PR (approvals will persist after a commit).

/hold

@easyCZ easyCZ self-assigned this May 17, 2022
Fix lint errors
@aledbf
Copy link
Member Author

aledbf commented May 17, 2022

/werft run

👍 started the job as gitpod-build-aledbf-watcher.21
(with .werft/ from main)

Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@jenting
Copy link
Contributor

jenting commented May 18, 2022

/unhold

@roboquat roboquat merged commit d00cc47 into main May 18, 2022
@roboquat roboquat deleted the aledbf/watcher branch May 18, 2022 05:11
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production deployed Change is completely running in production release-note size/XL team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants