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

Feature/viper config #7

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Feature/viper config #7

wants to merge 12 commits into from

Conversation

Faheetah
Copy link
Collaborator

Doing some maintenance mostly. Highlights:

Caught up lint/vet even if the //ObjName comments are crummy
Added a noop auth as a default instead of LDAP, so it's easier to just pick up and try, and not dependent on any configuration
RethinkDB can be skipped on tests if needed, handy if you want to work a module that doesn't need it but can't spin it up off hand (would be good for travis jobs)
Fixed CORS so we can deploy the UI separately, this is the main change so I can rewrite the UI without having to bother with directory shares, just directly dev locally through yarn start
Reworked the docker image to just be from scratch, used a docker pipeline to build it with the _/golang image, huge win here switched to viper so we don't need near as much logic for env vars, and was able to remove dockerize without consequence as a result

I'd like to propose ditching the vendor/ folder, I added it on the .gitignore here because it was crazy trying to get vscode to even comprehend how many files changed just removing burntsushi/toml and adding viper. I think we can just glide up when we clone. The deps are pinned to commits in the lock so there's nowhere near enough risk that they will get out of sync either.

@allen13
Copy link
Owner

allen13 commented Mar 23, 2018

Having the vendor directory committed really helps with build stability. It's a little bit messy but avoids problems like the infamous nodejs pad left dependency that broke the internet. What I usually do is separate dependency changes into separate commits. Update dependencies -> glide update -> git add . -> git commit -m "updating depencies"

https://www.theregister.co.uk/2016/03/23/npm_left_pad_chaos/

@Faheetah
Copy link
Collaborator Author

Yeah I remember the pad left nonsense. I was planning on doing a separate commit for them if we did keep it. Figured I would mention it since it does make IDEs a lot clunkier (just removing toml and adding viper was 800 file +-) with both search and git updates. I can add them in another commit and un-ignore it. If everything else looks good I'll run another commit this weekend for updated vendor.

@Faheetah
Copy link
Collaborator Author

Accidentally broked the jwt logic. Reverted it to restore ability to use multiple auth types (I see we use header and query string). Cleaned up a bit more, fixed the 500 from an empty /alerts/count and easier to click rows in the UI. If everything looks good I'll push up vendor.

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