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

For launchpad, add apiversion middleware #7

Merged
merged 3 commits into from
Nov 21, 2017
Merged

Conversation

aodhom
Copy link
Contributor

@aodhom aodhom commented Nov 14, 2017

Follow on from https://github.com/Teamwork/launchpad/pull/231
@ready4god2513 as suggested moved the apiversion header to teamwork/middleware.

I'll update the other one if this is ok.

@codecov
Copy link

codecov bot commented Nov 14, 2017

Codecov Report

Merging #7 into master will increase coverage by 45.63%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #7       +/-   ##
===========================================
+ Coverage   32.66%   78.29%   +45.63%     
===========================================
  Files           9        4        -5     
  Lines         398      129      -269     
===========================================
- Hits          130      101       -29     
+ Misses        258       24      -234     
+ Partials       10        4        -6
Impacted Files Coverage Δ
contenttypeMiddleware/contentType.go 93.33% <0%> (-0.79%) ⬇️
staticMiddleware/static.go
ratelimitMiddleware/ratelimiter.go
auditMiddleware/audit.go
corsMiddleware/cors.go
logMiddleware/reqlog.go
securityMiddleware/security.go
httperrMiddleware/error.go 87.23% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6891cd4...2a49865. Read the comment docs.

// output server header (example format US BETA 6ba849de1881e0d859a45714462ccb6d5ee9f015)
// SERVICE_TARGET is PROD or BETA (set in travis env vars)
apiVersion := fmt.Sprintf("%s %s %s", viper.GetString("AWS_REGION"),
viper.GetString("SERVICE_TARGET"), viper.GetString("VERSION"))
Copy link
Contributor

Choose a reason for hiding this comment

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

It's really not a good idea to use viper in libraries; otherwise it is very difficult to see what gets referenced where in even fairly small apps. You should add options for it so it gets passed from the application (e.g. like here).

Would also be better to use http.Handler instead of http.HandlerFunc as argument & return value, as that can be used with echo.WrapMiddleware and some other tools (see same link for example).

And finally, it's a very good idea to add at least a basic tests for libraries like this; otherwise it's very difficult to work on, and very easy to make a simple mistake. I know this is very simple function, but still ... it would be especially useful for changing it later (otherwise you'd have to copy this code to an app first, and then copy it back, and then maybe fix and copy and copy and copy...).

Adding a test is very easy; here's an example..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks 👍 I'll get to it. As usual I wish I hadn't touched this issue at all now :P

Copy link
Contributor

Choose a reason for hiding this comment

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

haha, they're small things, and I don't necessarily mind looking at it myself :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha nah I meant the end-to-end change in launchpad etc.
I'll have a crack sure good practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aodhom I can jump on this to close off this and https://github.com/Teamwork/launchpad/pull/231 for you today if you'd like

Copy link
Contributor

Choose a reason for hiding this comment

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

BUILD_TARGET usually implies compile-time configuration. This is more runtime configuration.

RUNTIME_ENVIRONMENT might make the most sense, in terms of being technically correct. But it's also extra long.

In desk we use:

var version = "devel"

in main.go, then re-define that variable after building, before deployment. Which technically makes it a build-time configuration, but we're pretending it's run-time configuration.

I'm not saying this is ideal; just offering another data point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think SERVER_ENV makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

version in Desk is the git commit, that doesn't tell us if it's production, beta, etc. We use SENTRY_ENVIRONMENT for that (mostly for historic reasons).

Launchpad actually does the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SERVER_ENV is good with me 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, k8 exposes the server name, so once we are all on k8 we should use that as it not only tells the environment, but which instance.

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.

None yet

4 participants