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

add toggle for events-logger in observability-bundle configmap #270

Merged
merged 5 commits into from
Nov 18, 2024

Conversation

QuantumEnigmaa
Copy link
Contributor

Towards giantswarm/roadmap#3750

This PR adds the toggle for the events-logger in the observability-bundle configmap.

Checklist

  • Update changelog in CHANGELOG.md.
  • Follow deployment test procedure in the tests/manual_e2e directory and have a working branch.

@QuantumEnigmaa QuantumEnigmaa self-assigned this Nov 18, 2024
@QuantumEnigmaa QuantumEnigmaa requested a review from a team as a code owner November 18, 2024 14:52
@@ -59,11 +59,32 @@ func GenerateObservabilityBundleConfigMap(ctx context.Context, lc loggedcluster.
return v1.ConfigMap{}, errors.Errorf("unsupported logging agent %q", lc.GetLoggingAgent())
}

if observabilityBundleVersion.GE(semver.MustParse("0.10.0")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We still have a few 0.10.1 observability bundles. Do we not need this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh well no we do not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that this covers these cases right ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well no because the Idea was to not deploy the grafana agent before v0.10.0 but we dont run those anymore anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok added it back in a different way. All good for you ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well no, because we should not set the value AT all before V10 and now you're Always setting it. Might as well remove the condition as it's unused anyway now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok fine with me :)

@QuantumEnigmaa QuantumEnigmaa enabled auto-merge (squash) November 18, 2024 15:48
@QuantumEnigmaa QuantumEnigmaa merged commit cf254e6 into main Nov 18, 2024
7 checks passed
@QuantumEnigmaa QuantumEnigmaa deleted the add-events-logger-toggle branch November 18, 2024 15:49
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