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

Simplifies the matchLabels for Oneagent daemonset #672

Merged
merged 6 commits into from
Mar 30, 2022

Conversation

0sewa0
Copy link
Contributor

@0sewa0 0sewa0 commented Mar 28, 2022

Description

The match labels shouldn't change between deployment modes or the operator will throw errors in case the mode is changed on the dynakube and the oneagent daemonset will not change unless deleted by hand.

How can this be tested?

  1. Deploy the operator
  2. Deploy a dynakube with a mode that creates a oneagent daemonset
  3. Update the dynakube to switch to a different mode

The labels should change, but not the matchLabels

Checklist

  • Unit tests have been updated/added
  • PR is labeled accordingly

@0sewa0 0sewa0 added the bug Something isn't working label Mar 28, 2022
@0sewa0 0sewa0 requested a review from a team as a code owner March 28, 2022 09:02
@@ -111,7 +111,7 @@ func TestLogGarbageCollector_modificationDateOlderThanTwoWeeks(t *testing.T) {
})

t.Run("is true for timestamp 14 days in past", func(t *testing.T) {
isOlder := isOlderThanTwoWeeks(time.Now().AddDate(0, 0, -14))
isOlder := isOlderThanTwoWeeks(time.Now().AddDate(0, 0, -15))
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

creating a separate PR for it would be to much work at this point IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite get what the purpose of this change is, but I agree with @0sewa0, it's such a trivial change it doesn't really need its own PR. It's more like housekeeping

@chrismuellner chrismuellner added the oneagent Changes related to Oneagent label Mar 28, 2022
@0sewa0 0sewa0 requested a review from meik99 March 28, 2022 10:56
@@ -12,95 +12,74 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

const (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the diff looks scary, but I just refactored this according to the guidelines

Copy link
Contributor

Choose a reason for hiding this comment

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

nicely refactored

Comment on lines +188 to +191
// buildMatchLabels produces a set of labels that
// don't change when switching between oneagent modes
// or during operator version update
// as matchLabels are not mutable on a Daemonset
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this comment, it is good, well written, and useful

src/kubeobjects/daemonset.go Outdated Show resolved Hide resolved
src/kubeobjects/daemonset.go Outdated Show resolved Hide resolved
src/kubeobjects/daemonset_test.go Show resolved Hide resolved
@@ -12,95 +12,74 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

const (
Copy link
Contributor

Choose a reason for hiding this comment

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

nicely refactored

@chrismuellner chrismuellner changed the title Simplifies the matchLabels Simplifies the matchLabels for Oneagent daemonset Mar 29, 2022
@0sewa0 0sewa0 enabled auto-merge (squash) March 30, 2022 10:22
@0sewa0 0sewa0 merged commit f34994e into master Mar 30, 2022
@0sewa0 0sewa0 deleted the bugfix/machLabels-conflict branch March 30, 2022 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working oneagent Changes related to Oneagent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants