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

remove config package-scoped variables #342

Merged
merged 9 commits into from
Feb 21, 2020

Conversation

crandles
Copy link
Contributor

@crandles crandles commented Dec 5, 2019

As part of supporting config reloading (#12), this PR aims to remove package-scoped global variables that are utilized throughout trickster. These are generally hard to maintain if they require post-init updates, and would all require some sort of locking or atomic operations for coordinating usage.

This will not attempt to add config reloading functionality, but should prepare the codebase for it 🤞.

https://peter.bourgon.org/blog/2017/06/09/theory-of-modern-go.html
https://dave.cheney.net/2017/06/11/go-without-package-scoped-variables

TO DO:

  • Remove log singleton
  • Remove global state required for binding flags
  • fix tests
  • remove comments

Test failures don't seem to be returned through travis; I may look at that as well.

@jranson
Copy link
Member

jranson commented Dec 5, 2019

re: the test failures, if I had just one guess, the cause of the always 'good' test return code is likely because the make test is being piped to a grep that strips log line entries from the output (such that only test results are printed). I would guess that the grep returns exit code 0 even if the make test returns a nonzero exit code, so we may need to pull that out. We could consider another option like setting the logger to be file-based and pointed at /dev/null, instead of console, for tests (it would be a lot of tests to update though!)

@crandles crandles force-pushed the global-state branch 2 times, most recently from 04c177e to 76a139b Compare December 20, 2019 15:55
}
}
// TODO: this fails.. the default config has a url set, unsure why this would error
// func TestLoadConfigurationMissingOriginURL(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this fails for me; there is a default that gets added by the Loader. I think this fails on next too.

@jranson jranson changed the base branch from next to master January 17, 2020 21:14
@jranson
Copy link
Member

jranson commented Jan 18, 2020

@crandles We've merged next into master, so I rebased your PR target here and next is no more. Currently all tests are passing on the master, so if you can rebase locally and fix up any compile failures as a result of the obscene amount of changes, it should be good to go. This is looking really good, thanks! Also, the fix is in for unit tests output and exit code working like it should, while also excluding generated code from the coverage report. I accomplished this by moving the exclusion sed from the Makefile target to the .travisci configuration, so you should be able to see all of the things now locally and in travis. Sorry for the frustrations with that!

@jranson jranson changed the base branch from master to v1.1.x February 19, 2020 14:31
@jranson jranson changed the title remove package-scoped variables remove config package-scoped variables Feb 20, 2020
@jranson jranson marked this pull request as ready for review February 21, 2020 13:54
@jranson jranson merged commit 000ecd2 into trickstercache:v1.1.x Feb 21, 2020
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.

2 participants