Skip to content

Bug: Fix vtadmin resources#358

Merged
GuptaManan100 merged 2 commits into
planetscale:mainfrom
nianfei97:vtadmin-resources
Jan 18, 2023
Merged

Bug: Fix vtadmin resources#358
GuptaManan100 merged 2 commits into
planetscale:mainfrom
nianfei97:vtadmin-resources

Conversation

@nianfei97
Copy link
Copy Markdown
Contributor

Currently, the resources requests/limits for the vtadmin component is not reflected in the spec for vtadmin

This is because the resource spec is copied to a local variable and discarded instead of the actual container spec

Signed-off-by: Teh Nian Fei tehnianfei@gmail.com

@notfelineit notfelineit requested review from notfelineit and removed request for GuptaManan100 and frouioui January 13, 2023 18:24
Copy link
Copy Markdown
Contributor

@GuptaManan100 GuptaManan100 left a comment

Choose a reason for hiding this comment

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

Yep, this looks like a bug. Thankyou for catching this. Could you add a test for this in the vtorc-vtadmin test that we have. You can change the config for the spec of vtadmin and verify in the test if the resources match the ones set.

Signed-off-by: Teh Nian Fei <tehnianfei@gmail.com>
@nianfei97
Copy link
Copy Markdown
Contributor Author

I have added a test to ensure that the vtadmin resources are set. Unfortunately, I couldn't seem to get the end2end tests running on my system, so I was unable to verify that the test works as expected.

@GuptaManan100
Copy link
Copy Markdown
Contributor

@nianfei97 I verified running the test locally, it works just as expected. There was a bug fix which was merged into main recently which is required to get the CI to pass, so I have merged main into the PR. Once the tests pass I shall merge the PR.

@GuptaManan100 GuptaManan100 merged commit e092ebf into planetscale:main Jan 18, 2023
@GuptaManan100
Copy link
Copy Markdown
Contributor

Thank-you for finding the bug and for your contributions @nianfei97! 🚀

@nianfei97 nianfei97 deleted the vtadmin-resources branch January 20, 2023 02:04
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