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

Add more tests around config parsing changes from common config PR #4433

Merged
merged 6 commits into from
Oct 12, 2021

Conversation

trevorwhitney
Copy link
Collaborator

What this PR does / why we need it:
Adds a bit more testing around the new config unmarshalling methods to confirm #4419 was not a real bug. It turned out just to be a strange behavior when passing a completely empty yaml file as the config file, like:

---

This was being used for tests, but is unlikely a case we need to solve for in real usage.

Which issue(s) this PR fixes:
Fixes #4419

Checklist

  • Documentation added
  • Tests updated

@trevorwhitney trevorwhitney requested a review from a team as a code owner October 7, 2021 18:50
@trevorwhitney trevorwhitney changed the base branch from main to populate-memberlist October 7, 2021 18:51
@dannykopping
Copy link
Contributor

@trevorwhitney can we name this PR a little more descriptively please? We use the PR names for our changelogs

@trevorwhitney trevorwhitney changed the title Config parse bug fix 2 Add more tests around config parsing changes that were introduced common config PR Oct 7, 2021
@trevorwhitney
Copy link
Collaborator Author

@dannykopping yes, absolutely, sorry I didn't mean to actually name it after that terrible commit!

@dannykopping
Copy link
Contributor

@dannykopping yes, absolutely, sorry I didn't mean to actually name it after that terrible commit!

Haha no sweat! Thanks 🙌

@trevorwhitney trevorwhitney changed the title Add more tests around config parsing changes that were introduced common config PR Add more tests around config parsing changes from common config PR Oct 7, 2021
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@trevorwhitney
Copy link
Collaborator Author

Going to wait on resolving conflicts until #4400 is merged as this builds on that branch

Base automatically changed from populate-memberlist to main October 7, 2021 22:06
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
…here

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>
@trevorwhitney
Copy link
Collaborator Author

@dannykopping this should be good to go now, would you mind merging if you have permission to do so?

@dannykopping dannykopping merged commit 81d0f1d into main Oct 12, 2021
@dannykopping dannykopping deleted the config-parse-bug-fix-2 branch October 12, 2021 16:26
@dannykopping
Copy link
Contributor

You got it 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: unexpected behavior around defaults when unmarshalling config
2 participants