-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[cleanup] Remove kedge #10292
[cleanup] Remove kedge #10292
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.
LGTM. I'd recommend staging the removal. We need to get into the habit of not changing application and configuration and expect them to be in perfect sync always.
Up to you if you want to tackle the comments, but I'd recommend it.
/hold
operations/observability/mixins/meta/dashboards/components/meta-services.json
Show resolved
Hide resolved
@@ -29,7 +29,6 @@ type Components struct { | |||
} `json:"imageBuilderMk3"` | |||
InstallationTelemetry Versioned `json:"installationTelemetry"` | |||
IntegrationTests Versioned `json:"integrationTests"` | |||
Kedge Versioned `json:"kedge"` |
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.
FWIW, I'd recommend renaming this to _
to make it unused in the first phase of removal. Config can be really nasty and sometimes you've got old versions of config that do define this field. We'll fail to parse the json/yaml when there are fields which are not defined in the struct.
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.
got old versions of config that do define this field
Two comments on this:
- this is the versions file. We don't use it as config, plus no-one should do so
- if this were a relevant config field: instead of manually managing obsolescence of single fields, which involves multiple PRs, managing phase-out periods and such, we should instead make our parsing behavior in installer less picky (e.g., with a "warn but continue anyway" flag). The current hard lock-in between a) installer binary, b) versions file and c) config is not flexible enough for RL usecases.
We should be cautious where it make sense, but also watch out for our pace. 🙂
This has not been used actively since February, and definitely not deployed since our installer migration. (Updated the description). |
/unhold |
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, only looked at my files
Description
Removes all code and links to kedge.
This has not been used actively since February, and definitely not deployed since our installer migration 2 weeks ago.
Related Issue(s)
Fixes #9487
How to test
Release Notes
Documentation