Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Merged by Bors] - allow specifying pvc parameters #164

Closed
wants to merge 31 commits into from

Conversation

razvan
Copy link
Member

@razvan razvan commented Apr 16, 2022

Description

Fixes #67

Review Checklist

  • Code contains useful comments
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)
  • Cargo.toml only contains references to git tags (not specific commits or branches)
  • Helm chart can be installed and deployed operator works (or not applicable)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

@razvan razvan requested a review from a team April 16, 2022 20:15
@razvan razvan linked an issue Apr 16, 2022 that may be closed by this pull request
3 tasks
@nightkr
Copy link
Member

nightkr commented Apr 19, 2022

@soenkeliebau has been working on integrating something similar into his resource management scheme (over at https://github.com/stackabletech/operator-rs/compare/feat/resource_config2, don't think he made a proper PR from what I can tell?). Might be worth talking to him about aligning this.

@razvan
Copy link
Member Author

razvan commented Apr 19, 2022

@soenkeliebau has been working on integrating something similar into his resource management scheme (over at https://github.com/stackabletech/operator-rs/compare/feat/resource_config2, don't think he made a proper PR from what I can tell?). Might be worth talking to him about aligning this.

Didn't know that. Thanks for the pointer.

Copy link
Member

@maltesander maltesander left a comment

Choose a reason for hiding this comment

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

Just some comments. Ill test tomorrow. Can we have a readme for the kuttl tests? Its not explained how to properly run that with the test definition yaml?

razvan and others added 3 commits May 8, 2022 18:50
Co-authored-by: Malte Sander <contact@maltesander.com>
Co-authored-by: Malte Sander <contact@maltesander.com>
Co-authored-by: Malte Sander <contact@maltesander.com>
Copy link
Member

@maltesander maltesander left a comment

Choose a reason for hiding this comment

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

Tested and works. @soenkeliebau should approve once he had a chance to look over this.
Locally the tests failed the first time (timeout 5min) and worked after the images were downloaded. Maybe we can increase the timeout for the install step?

@maltesander maltesander requested a review from soenkeliebau May 9, 2022 08:49
Copy link
Member

@soenkeliebau soenkeliebau left a comment

Choose a reason for hiding this comment

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

I have looked it over and tested it, looks good to me.
One thing I noticed is that the tests specify resources but don't check if they get set in the pods / pod templates correctly. That migth be an improvement down the line that we could add.
But I am also happy for this to go in as is.

@razvan
Copy link
Member Author

razvan commented May 17, 2022

bors merge

bors bot pushed a commit that referenced this pull request May 17, 2022
@bors
Copy link
Contributor

bors bot commented May 17, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title allow specifying pvc parameters [Merged by Bors] - allow specifying pvc parameters May 17, 2022
@bors bors bot closed this May 17, 2022
@bors bors bot deleted the 67-allow-specifying-pvc-parameters branch May 17, 2022 07:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make resources configurable (CPU, memory, PVCs)
4 participants