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

Set the cats-config defaults and missing values #1291

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

iaftab-alam
Copy link
Contributor

@iaftab-alam iaftab-alam commented Oct 29, 2024

Are you submitting this PR against the develop branch?

Yes

What is this change about?

Fix typo in the example-cats-config.json and created a new complete-cats-config.json file with all available options and set the defaults correctly.

Please provide contextual information.

Some tests have been improved and new configuration options have been introduced. Similarly, not all options are clearly visible in the README.md. Therefore, created a new complete-cats-config.json file with all available options and set the defaults correctly.

What version of cf-deployment have you run this cf-acceptance-test change against?

N/A

Please check all that apply for this PR:

  • introduces a new test --- Are you sure everyone should be running this test?
  • changes an existing test
  • requires an update to a CATs integration-config

Did you update the README as appropriate for this change?

  • YES
  • N/A

If you are introducing a new acceptance test, what is your rationale for including it CATs rather than your own acceptance test suite?

N/A

How many more (or fewer) seconds of runtime will this change introduce to CATs?

N/A

What is the level of urgency for publishing this change?

  • Urgent - unblocks current or future work
  • Slightly Less than Urgent

Tag your pair, your PM, and/or team!

It's helpful to tag a few other folks on your team or your team alias in case we need to follow up later.

Copy link
Contributor

@jochenehret jochenehret left a comment

Choose a reason for hiding this comment

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

Ok now the file has become quite big. I didn't realize that we have so many options. Perhaps we should add this file as an additional complete-cats-config.json? Any opinions?

@iaftab-alam
Copy link
Contributor Author

yes! It make sense, suggesting the complete-cats-config.json file for all CATs options is a great idea! It will provide a comprehensive reference for all CATs configurations in one place without digging through code to find the default values.

* complete-cats-config.json provide a comprehensive reference
for all CATs configurations in one place without digging
through code to find the default values.

* fix type for readiness_health_checks_enabled in example-cats-config.json
Copy link
Contributor

@jochenehret jochenehret left a comment

Choose a reason for hiding this comment

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

"IN DEVELOPMENT" can be removed after this overhaul. Let's wait a few more days to give everyone a chance for review.

complete-cats-config.json Outdated Show resolved Hide resolved
@jochenehret jochenehret requested review from a team October 29, 2024 13:25
@davewalter
Copy link
Member

yes! It make sense, suggesting the complete-cats-config.json file for all CATs options is a great idea! It will provide a comprehensive reference for all CATs configurations in one place without digging through code to find the default values.

Are we concerned that this file will get out of date as we modify the configuration in the future? I am especially concerned about the default values changing and folks not realising that the sample file is wrong.

@iaftab-alam
Copy link
Contributor Author

yes! It make sense, suggesting the complete-cats-config.json file for all CATs options is a great idea! It will provide a comprehensive reference for all CATs configurations in one place without digging through code to find the default values.

Are we concerned that this file will get out of date as we modify the configuration in the future? I am especially concerned about the default values changing and folks not realising that the sample file is wrong.

“Good point! To help avoid drift and make sure the file remains up-to-date, I suggest making it part of the PR process to verify this JSON file and make sure all the values are up-to-date. Adding something a checklist item in PRs template or even a checker on PR to verify if configuration changes is impacting this json file. Lastly, any of the approvers can also check this [manually] and enforce the policy.

@davewalter
Copy link
Member

“Good point! To help avoid drift and make sure the file remains up-to-date, I suggest making it part of the PR process to verify this JSON file and make sure all the values are up-to-date. Adding something a checklist item in PRs template or even a checker on PR to verify if configuration changes is impacting this json file. Lastly, any of the approvers can also check this [manually] and enforce the policy.

My preference would be to have a small Golang utility that could generate a complete config.json on demand from the existing defaults in the code. Users could then run it and modify the resulting JSON file as desired for their environment. I feel like adding a manual verification/enforcement step to the PR process is not going to be enough.

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