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

feat: Validate Collection memory config through adding conflictsWith validation #806

Merged
merged 5 commits into from
Jul 31, 2023

Conversation

MasonLegere
Copy link
Contributor

@MasonLegere MasonLegere commented Jul 24, 2023

Which problem is this PR solving?

Fixes #803 by ensuring that only both AvailableMemory and MaxAlloc cannot be set at the same. MaxMemoryPercentage may still be set regardless as it has a default value defined.

While making this changes I noticed that when only AvailableMemory was defined I ran into a validation requireWith error. The validation only fails on the first pass before defaults are loaded in from the config struct. If the failure is intentional then we probably should remove the default -- only breaking change I could see is if someone was running with validations turned off explicitly.

 $ docker-compose up
[+] Running 1/0
 ✔ Container refinery-refinery-1  Recreated                                                                   0.0s
Attaching to refinery-refinery-1
refinery-refinery-1  | 2023/07/24 07:57:40 maxprocs: Leaving GOMAXPROCS=6: CPU quota undefined
refinery-refinery-1  | Validation failed for config file /etc/refinery/config.yaml:
refinery-refinery-1  |   the group Collection includes AvailableMemory, which also requires MaxMemoryPercentage
refinery-refinery-1  |
refinery-refinery-1  |
refinery-refinery-1 exited with code 1
$ cat config.yaml 
General:
  ConfigurationVersion: 2
  MinRefineryVersion: v2.0


Collection:
  AvailableMemory: 2Gb

Short description of the changes

  • Adds conflictsWith validation type acting in an opposite way to requireWith
  • Modified requireWith to check whether or not the required field has a default value

Fixes #803.

@MasonLegere MasonLegere requested a review from a team as a code owner July 24, 2023 08:22
@MasonLegere MasonLegere changed the title Memory validation feat: Validate Collection memory config through adding conflictsWith validation Jul 24, 2023
@kentquirk kentquirk added this to the v2.1 milestone Jul 27, 2023
Copy link
Contributor

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

This is good, thanks. Minor tweaks, please.

@kentquirk kentquirk merged commit c088a29 into honeycombio:main Jul 31, 2023
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.

[config] Validation should fail if MaxAlloc, AvailableMemory, and MaxMemoryPercentage are all set
2 participants