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

testsys: Provide advanced config control #2799

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

ecpullen
Copy link
Contributor

Issue number:

N/A

Description of changes:

When developing TestSys, the simplicity of cargo make test calls can make it difficult to force certain behavior to occur. It is also possible that the default values are not the best for a user. This pr introduces DeveloperConfig for additional control.

DeveloperConfig is intended to contain advanced configuration values that most users shouldn't need to care about. There is currently a 1:1 mapping for GenericVariantConfig : Command Line Env Variables that I'd like to preserve which is why I created a new type.

Currently there are 4 configuration values:

  • cluster-destruction-policy allows users to control the destruction policy of cluster crds. The defaults currently used were carefully selected to improve the ux, but some users may want to alter this behavior
  • bottlerocket-destruction-policy allows users to control the destruction policy of Bottlerocket crds.
  • keep-tests-running is used to set the keep_running field on tests. With keep_running: true, the test's pods are kept in a running state until the test is deleted, as a result, the default behavior has been set to false, but can be enabled if a user wishes to.
  • image-account-id is used to set an alternate account as the one starting AMIs are stored in. This is useful for storing AMIs in a separate account from the one testing is occurring in. Without image-account-id, a user needs to find the ami id from the account and set TESTSYS_STARTING_IMAGE_ID. With this new variable, all manual steps are removed.

Testing done:

Tested each new value and verified that the expected values were added to the CRDs.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@ecpullen ecpullen requested a review from etungsten February 11, 2023 00:07
Copy link
Contributor

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

Can we get a small section added to TESTING.md that documents this?

@ecpullen
Copy link
Contributor Author

Can we get a small section added to TESTING.md that documents this?

The dev configurations were purposely left out of TESTING.md to keep it from getting too long and keep it simple. I prefer to keep these configuration values documented in the code base instead to prevent accidental usage.

@ecpullen ecpullen requested a review from stmcginnis February 13, 2023 22:25
Copy link
Contributor

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

I would prefer to see some documentation somewhere other than the code, but makes sense. Everything else looks good to me.

@ecpullen ecpullen force-pushed the testsys-developer-config branch from 58779b0 to f5e0d4f Compare February 13, 2023 22:39
@ecpullen
Copy link
Contributor Author

Added a note in Test.toml.example for the new fields.

@ecpullen ecpullen requested a review from stmcginnis February 13, 2023 22:40
Copy link
Contributor

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @ecpullen!

@ecpullen ecpullen merged commit 4a5b78e into bottlerocket-os:develop Feb 14, 2023
@ecpullen ecpullen deleted the testsys-developer-config branch February 14, 2023 18:36
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.

3 participants