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

refactor: startup config validation #120

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

lvlcn-t
Copy link
Member

@lvlcn-t lvlcn-t commented Mar 3, 2024

Motivation

Currently the target manager config has a Validate method that is never consumed.

This PR is part of #66 which will implement another target manager type and therefore needs more target manager config validation. That's also when I noticed that the target manager config is never validated. 😄

Changes

I've refactored the startup config validation so every startup config component has its own Validate() method.

  • fix: validate target manager config
  • refactor: split config validation in smaller methods
  • chore: use errors instead of ok booleans

For additional information look at the commits.

Tests done

I've adjusted the existing unit tests.

  • Unit tests succeeded
  • E2E tests succeeded

TODO

  • I've assigned this PR to myself
  • I've labeled this PR correctly

* fix: validate target manager config
* refactor: split config validation in smaller methods
* chore: use errors instead of ok booleans
@lvlcn-t lvlcn-t added refactoring Refactoring of existing code area/config Issues/PRs related to the startup/sparrow config area labels Mar 3, 2024
@lvlcn-t lvlcn-t self-assigned this Mar 3, 2024
@lvlcn-t
Copy link
Member Author

lvlcn-t commented Mar 3, 2024

The actions are failing because the PR comes from my fork.

All checks are succeeding here, where I've implemented the whole issue of #66 (also with the changes of this PR): https://github.com/caas-team/sparrow/tree/feat/git-tarman

Copy link
Member

@puffitos puffitos left a comment

Choose a reason for hiding this comment

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

LGTM. I don't appreciate the log in the validate function, that's only code we'll forget to update when the logic changes. We should just return a (structured) error. The standardized error text is OK for now.

Thanks for adding the godocs as well and refactoring the tests :)

@lvlcn-t
Copy link
Member Author

lvlcn-t commented Mar 5, 2024

@puffitos I agree with you, maybe we should change that in the future to returning to a structured error like this:

// ErrInvalidConfig is an error for invalid configuration
type ErrInvalidConfig struct {
	Field string
	Err   error
}

// Error returns the error message
func (e ErrInvalidConfig) Error() string {
	return fmt.Sprintf("invalid config for %q: %v", e.Field, e.Err)
}

But since it makes no difference for the user, I'll implement this in another PR in the near future.
(I want to merge this to continue opening PR's for #66 😄 )

@lvlcn-t lvlcn-t merged commit 3b51b9d into caas-team:main Mar 5, 2024
3 of 6 checks passed
@lvlcn-t lvlcn-t deleted the refactor/config-validation branch March 5, 2024 18:09
@puffitos
Copy link
Member

puffitos commented Mar 5, 2024

@lvlcn-t the struct seems appropriate. The #111 will take priority in this iteration, just FIY.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config Issues/PRs related to the startup/sparrow config area refactoring Refactoring of existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants