Skip to content
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

Proxy Mist triggers from catalyst to catalyst-api and remove dynamic Mist Trigger Setup #1362

Merged
merged 2 commits into from
Aug 21, 2024

Conversation

leszko
Copy link
Contributor

@leszko leszko commented Aug 21, 2024

I always thought that catalyst-api dynamically sets Mist triggers. This is true, but this config gets overwritten with the default config present here every time MistController restarts of the config file is changed. The only reason it worked in prod is that our dynamic Mist Trigger configuration is exactly the same as the config map configuration.

I think it's super misleading and it led to the issues when I tried to use the standalone catalyst-api, because my config got overwritten and all stopped working at some point.


First of all, I suggest removing this whole dynamic mist triggers setup (to avoid further confusion). Secondly, we need to think how to solve it if catalyst-api is deployed separately.

  1. Option 1: What I implemented in this PR => Proxy each Mist trigger request from catalyst to catalyst-api
  2. Option 2: Monitor Mist triggers and dynamically override them each time the config changes
  3. Option 3: Investigate it further, maybe if we remove the Mist triggers configuration from here it will not override the dynamically set configuration; the reason I didn't start from this option is that I'm afraid we'll come to some corner cases like Mist crashing or something and then we'll end up with Mist with no triggers set up at all
  4. Option 4: Have a separate config map for each catalyst pod (then we can differentiate between triggers in the infra config)

@leszko leszko requested review from thomshutt and mjh1 August 21, 2024 08:20
@mjh1
Copy link
Contributor

mjh1 commented Aug 21, 2024

Could you link to the block(s) of code for the dynamic triggers setup which in theory could be remove? Or are you saying you've removed those bits already in this PR?
This idea of proxying doesn't seem too bad since we're already having to do that for some events right? (implemented in #1347)

@leszko
Copy link
Contributor Author

leszko commented Aug 21, 2024

Could you link to the block(s) of code for the dynamic triggers setup which in theory could be remove? Or are you saying you've removed those bits already in this PR? This idea of proxying doesn't seem too bad since we're already having to do that for some events right? (implemented in #1347)

It's already removed in this PR. Yeah, I think the proxying is ok as a hack 🙃

@leszko leszko merged commit b6576e7 into main Aug 21, 2024
11 checks passed
@leszko leszko deleted the rafal/standalone-catalyst-api-mist-triggers branch August 21, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants