Skip to content
This repository was archived by the owner on Sep 12, 2018. It is now read-only.

Conversation

dmp42
Copy link
Contributor

@dmp42 dmp42 commented Aug 7, 2014

@shin- @wking @bacongobbler @noxiouz @lyda

This is the last round of changes for 0.8

There really isn't much to it (commits are rather self-explicit), except adding new relic support.

Please review, and if possible, give a spin to the future 0.8 - simplest way is to build the container - otherwise, be sure to:

cd depends/docker-registry-core/
python setup.py develop
cd -
python setup.py develop

as registry-core is not on pypi yet.

Olivier Gambier added 6 commits August 7, 2014 15:34
Introduce two new environment configuration variables: NEW_RELIC_INI and NEW_RELIC_STAGE
Added new extra "newrelic" installation opt-in.

Docker-DCO-1.1-Signed-off-by: Olivier Gambier <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Olivier Gambier <[email protected]> (github: dmp42)
Removed no more existing package and adding missing on.
Upped cover-min to what we currently have (67%).

Docker-DCO-1.1-Signed-off-by: Olivier Gambier <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Olivier Gambier <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Olivier Gambier <[email protected]> (github: dmp42)
Docker-DCO-1.1-Signed-off-by: Olivier Gambier <[email protected]> (github: dmp42)
@dmp42 dmp42 added this to the 0.8 milestone Aug 7, 2014
@wking
Copy link
Contributor

wking commented Aug 7, 2014

On Thu, Aug 07, 2014 at 08:37:25AM -0700, Olivier Gambier wrote:

  • New relic support

This commit uses env.source('NEW_RELIC_INI') twice. You just use the
already-extracted newini when calling newrelic.agent.initialize().
Also, I'd prefer _new_relic_ini as the variable name. It's private
(hence the underscore) and lowercase (for PEP 8). Otherwise I see no
reason to diverge from the environment variable name.

I'd also prefer a trailing comma in setup.py's extras_require. Then
you don't have to touch the newrelic line when you add another extra
(like you had to touch the bugsnag line here).

  • Loose dev requirements

Yay :). Although it looks like you're still not trusting these
packages to use semantic versioning ;). But I'll take what I can get
:p.

  • Python compat

Why move the ‘ver’ assignment here? And I imagine these are for
Python 2.6, but saying that explicitly in the commit message would be
nice.

Otherwise these look good to me.

@dmp42
Copy link
Contributor Author

dmp42 commented Aug 8, 2014

Good catches. Must have been sleepy coding - will amend.

re: python compat - this is for python3 actually, but you are right.

- don't source NEW_RELIC_INI multiple times
- better variable name for new relic ini ref
- learn to stop worrying about dev dependencies
- trailing coma in extras section of setup to avoid changing multiple lines when adding a new one

Docker-DCO-1.1-Signed-off-by: Olivier Gambier <[email protected]> (github: dmp42)
@dmp42
Copy link
Contributor Author

dmp42 commented Aug 9, 2014

You got your semver zen, mister :-)

@dmp42
Copy link
Contributor Author

dmp42 commented Aug 10, 2014

@shin- ping

@wking
Copy link
Contributor

wking commented Aug 10, 2014

On Sat, Aug 09, 2014 at 01:13:38AM -0700, Olivier Gambier wrote:

You got your semver zen, mister :-)

6a580a2 (Amending previous commits, 2014-08-09) looks good to me.
Any chance I can get it split and squashed into the original commits?
;). It will make for easier archaeology later and easier reviewing
for @shin- now ;).

@shin-
Copy link
Contributor

shin- commented Aug 11, 2014

LGTM! :shipit:

dmp42 added a commit that referenced this pull request Aug 11, 2014
@dmp42 dmp42 merged commit 5f965db into master Aug 11, 2014
@dmp42 dmp42 deleted the next-release branch August 11, 2014 16:56
@dmp42
Copy link
Contributor Author

dmp42 commented Aug 12, 2014

Fixes #447 and fixes #511

@silarsis
Copy link
Contributor

Quick question - I know this is closed (and in production), but new relic supports calling newrelic.agent.initialize() without any arguments and leaning on (NEW_RELIC_CONFIG_FILE and NEW_RELIC_EVIRONMENT) or NEW_RELIC_LICENSE_KEY. Is there a reason that docker-registry uses different environment variable names to achieve the same thing?

If the standard names were used (ie. just call initialize() without args if any of the three env variables existed), it would allow us to pass in a license key only without having to bake a new version of the registry that has an extra config file...

https://docs.newrelic.com/docs/agents/python-agent/customization-extension/python-management-api#initialize

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

Successfully merging this pull request may close these issues.

4 participants