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

Limit max concurrent jobs by type (prune, backup, check, etc.) #164

Merged
merged 5 commits into from
Dec 2, 2020

Conversation

susana-garcia
Copy link
Contributor

Fixes #120

@susana-garcia
Copy link
Contributor Author

I still need to refactor the constants bit with the new koanf configuration part, after that PR is merged

@susana-garcia susana-garcia force-pushed the feat_limit_concurrent_jobs branch from 8691dac to afc0324 Compare December 1, 2020 09:39
@susana-garcia susana-garcia marked this pull request as ready for review December 1, 2020 09:40
@susana-garcia susana-garcia requested a review from ccremer December 1, 2020 09:41
@ccremer
Copy link
Contributor

ccremer commented Dec 1, 2020

PR #155 is now merged, you could do a rebase so that the diff is more clear :)

@susana-garcia
Copy link
Contributor Author

@ccremer already done :)

executor/worker.go Outdated Show resolved Hide resolved
observer/observer.go Outdated Show resolved Hide resolved
observer/observer.go Outdated Show resolved Hide resolved
observer/observer.go Outdated Show resolved Hide resolved
@susana-garcia susana-garcia force-pushed the feat_limit_concurrent_jobs branch from 0e83241 to ccc2f58 Compare December 1, 2020 10:13
@ccremer
Copy link
Contributor

ccremer commented Dec 1, 2020

Can you add some unit tests as well? We should really start with those :)

@susana-garcia
Copy link
Contributor Author

susana-garcia commented Dec 1, 2020

@ccremer I'm adding some unit tests to test the observer's methods, is that ok?

@susana-garcia
Copy link
Contributor Author

@ccremer @Kidswiss also I forgot to ask: for the tests we want to have BDD testing? I saw that we're already using ginkgo or are you thinking on something else?

observer/observer.go Outdated Show resolved Hide resolved
queue/execution.go Outdated Show resolved Hide resolved
@ccremer
Copy link
Contributor

ccremer commented Dec 1, 2020

@ccremer I'm adding some unit tests to test the observer's methods, is that ok?

Yes, you can limit tests for the affected change in this PR only

@ccremer
Copy link
Contributor

ccremer commented Dec 1, 2020

@ccremer @Kidswiss also I forgot to ask: for the tests we want to have BDD testing? I saw that we're already using ginkgo or are you thinking on something else?

I think for this change it's sufficient to create classic unit tests with testing.T. I don't think you need the in-memory K8s API server to test the limits, unless it's very hard to mock/stub the dependent functions

observer/observer.go Outdated Show resolved Hide resolved
@susana-garcia susana-garcia force-pushed the feat_limit_concurrent_jobs branch from 441f495 to 2fffa87 Compare December 2, 2020 08:43
@susana-garcia
Copy link
Contributor Author

@ccremer @Kidswiss I think it's ready now, but please review it again.

cfg/config.go Outdated Show resolved Hide resolved
observer/observer.go Outdated Show resolved Hide resolved
observer/observer.go Outdated Show resolved Hide resolved
@susana-garcia susana-garcia force-pushed the feat_limit_concurrent_jobs branch 2 times, most recently from 55951c2 to 857d7c9 Compare December 2, 2020 09:17
@susana-garcia
Copy link
Contributor Author

@ccremer @Kidswiss please review it again when you have a moment.

observer/observer_test.go Outdated Show resolved Hide resolved
@susana-garcia susana-garcia force-pushed the feat_limit_concurrent_jobs branch from 1e560c9 to 15612b5 Compare December 2, 2020 09:56
@susana-garcia
Copy link
Contributor Author

@ccremer I think it's ready

Copy link
Contributor

@ccremer ccremer left a comment

Choose a reason for hiding this comment

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

I cannot really judge whether the business logic does what is needed to. I'll let @Kidswiss judge that, but code LGTM

@ccremer ccremer requested a review from Kidswiss December 2, 2020 10:16
@susana-garcia
Copy link
Contributor Author

@ccremer ok, thank you

observer/observer.go Show resolved Hide resolved
@susana-garcia susana-garcia force-pushed the feat_limit_concurrent_jobs branch from 4a0ffb1 to bb54af0 Compare December 2, 2020 12:14
@susana-garcia
Copy link
Contributor Author

@Kidswiss I think the PR is ready

@ccremer ccremer merged commit bde716c into master Dec 2, 2020
@ccremer ccremer deleted the feat_limit_concurrent_jobs branch December 2, 2020 18:43
@susana-garcia susana-garcia restored the feat_limit_concurrent_jobs branch December 3, 2020 08:54
@ccremer ccremer deleted the feat_limit_concurrent_jobs branch December 4, 2020 21:05
@ccremer ccremer added the enhancement New feature or request label Dec 30, 2020
@ccremer ccremer changed the title feat: limit max concurrent jobs by type (prune, backup, check, etc.) Limit max concurrent jobs by type (prune, backup, check, etc.) Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit max concurrent jobs
4 participants