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.

Sign up for GitHub

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

Jump to bottom

Support wpa configuration #142

Merged
merged 2 commits into from
Aug 31, 2020
Merged

Support wpa configuration #142

merged 2 commits into from
Aug 31, 2020

Conversation

ahmed-mez
Copy link
Contributor

What does this PR do?

Add a dedicated field in the CRD and the required rbacs for WPA

@ahmed-mez ahmed-mez added the enhancement New feature or request label Aug 28, 2020
@ahmed-mez ahmed-mez requested a review from a team as a code owner August 28, 2020 11:13
github-actions[bot]
github-actions bot approved these changes Aug 28, 2020
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

@ahmed-mez ahmed-mez added this to the v0.4 milestone Aug 28, 2020
github-actions[bot]
github-actions bot approved these changes Aug 28, 2020
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

github-actions[bot]
github-actions bot approved these changes Aug 28, 2020
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

clamoriniere
clamoriniere reviewed Aug 28, 2020
View reviewed changes
pkg/apis/datadoghq/v1alpha1/datadogagent_types.go Outdated
@@ -664,6 +664,11 @@ type ExternalMetricsConfig struct {
// +optional
Enabled bool `json:"enabled,omitempty"`

// Enable informer and controller of the watermark pod autoscaler
// NOTE: You need to install the `WatermarkPodAutoscaler` CRD before
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe something more generic: the WatermarkPodAutoscaler-Controller (https://github.com/DataDog/watermarkpodautoscaler) need to be install

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, done 👍

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2020 •
edited

Codecov Report

Merging #142 into master will increase coverage by 0.39%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   58.02%   58.42%   +0.39%     
==========================================
  Files          31       31              
  Lines        4610     4623      +13     
==========================================
+ Hits         2675     2701      +26     
+ Misses       1739     1726      -13     
  Partials      196      196
Flag Coverage Δ
#unittests 58.42% <100.00%> (+0.39%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/controller/datadogagent/clusteragent.go 75.47% <100.00%> (+0.72%) ⬆️
pkg/controller/datadogagent/service.go 82.86% <0.00%> (+4.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f2d252...d71c5ed. Read the comment docs.

github-actions[bot]
github-actions bot approved these changes Aug 28, 2020
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

github-actions[bot]
github-actions bot approved these changes Aug 28, 2020
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

L3n41c
L3n41c requested changes Aug 31, 2020
View reviewed changes
pkg/controller/datadogagent/datadogagent_controller_test.go Outdated Show resolved Hide resolved
pkg/controller/datadogagent/datadogagent_controller_test.go
Comment on lines +2188 to +2190
if reflect.DeepEqual(policyRule.Verbs, requiredVerbs) {
verbsFound = true
}
Copy link
Member

Choose a reason for hiding this comment

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

This will make the function return false in the cases where we have more privileges than expected (i.e.: not only get, list and watch but also something else or even *.)

Copy link
Contributor Author

@ahmed-mez ahmed-mez Aug 31, 2020 •
edited

Choose a reason for hiding this comment

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

yes that's true, it's a util function for a test case though I wanted to leave it as dumb as possible - I think we can keep it like this and extend it if needed in other test cases in the future

github-actions[bot]
github-actions bot approved these changes Aug 31, 2020
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

@ahmed-mez @L3n41c
make hasWpasRbacs more restrictive
d71c5ed 
Co-authored-by: Lénaïc Huard <L3n41c@users.noreply.github.com>
github-actions[bot]
github-actions bot approved these changes Aug 31, 2020
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

@clamoriniere clamoriniere merged commit 9c1fce9 into master Aug 31, 2020
@clamoriniere clamoriniere deleted the ahmed-mez/support-wpa branch August 31, 2020 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clamoriniere clamoriniere clamoriniere approved these changes

@L3n41c L3n41c L3n41c approved these changes

@github-actions github-actions[bot] github-actions approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ahmed-mez @codecov-commenter @clamoriniere @L3n41c