feat(api): inject release metadata into application#1080
feat(api): inject release metadata into application#1080bacongobbler merged 1 commit intodeis:masterfrom
Conversation
|
@helgi, @kmala and @Joshua-Anderson are potential reviewers of this pull request based on my analysis of |
|
Is this what the ticket that was created asked for? Thought there was a workflow prefix and all? |
|
Given deis/builder#418 and Heroku both use SOURCE_VERSION it makes sense to use that convention. I can add the other environment variables here, one sec |
|
Ah is that the only one that was not prefixed? Sounds good |
| port = release.get_port() | ||
| if port: | ||
| default_env['PORT'] = port | ||
| default_env['PORT'] = port |
There was a problem hiding this comment.
any specific reason for this change or just a clean up
There was a problem hiding this comment.
this is the rebased commit from #1078, but essentially this is cleanup from dead code. get_port used to return a NoneType if the port didn't exist; now it just raises a DeisException or a numerical value so we don't need this check any more
e88c52f to
ddfab9b
Compare
Current coverage is 87.12% (diff: 100%)@@ master #1080 diff @@
==========================================
Files 42 42
Lines 3601 3603 +2
Methods 0 0
Messages 0 0
Branches 609 610 +1
==========================================
+ Hits 3137 3139 +2
Misses 307 307
Partials 157 157
|
ddfab9b to
e3f4d33
Compare
| default_env['BUILDER_STORAGE'] = settings.APP_STORAGE | ||
| default_env['DEIS_MINIO_SERVICE_HOST'] = settings.MINIO_HOST | ||
| default_env['DEIS_MINIO_SERVICE_PORT'] = settings.MINIO_PORT | ||
| default_env['SOURCE_VERSION'] = release.build.sha |
There was a problem hiding this comment.
should we check for sha and add as environment as it is available even for dockerfile apps
There was a problem hiding this comment.
Sure, that's not a bad idea.
There was a problem hiding this comment.
FYI this is done now
7d8ba06 to
096948f
Compare
096948f to
644d3df
Compare
|
Seems like we should add these new vars to the test here: https://github.com/deis/controller/blob/master/rootfs/api/tests/test_app.py#L585 |
|
They have been added! See the changes to test_app.py in this PR. :) |
| 'WORKFLOW_RELEASE': 'v{}'.format(release.version), | ||
| 'WORKFLOW_RELEASE_SUMMARY': release.summary, | ||
| 'WORKFLOW_RELEASE_CREATED_AT': str(release.created.strftime( | ||
| settings.DEIS_DATETIME_FORMAT)) |
There was a problem hiding this comment.
Does this need to go through strftime? Just curious since we set the format already for DRF so I'd expect it to translate it for us when we do str() on it, but maybe not?
There was a problem hiding this comment.
I think it does because we do it elsewhere in the codebase. DRF just handles it in the serializer, I think.
|
Did we consider the ambiguousness that was brought up in #1057 and what was the final decision? I see we only cover the |
With this pr we now cover WORKFLOW_RELEASE_SUMMARY, WORKFLOW_RELEASE_CREATED_AT and SOURCE_VERSION which is WORKFLOW_RELEASE_HASH. I didn't bother with WORKFLOW_RELEASE_HASH because SOURCE_VERSION already has that data. |
|
@bacongobbler Thanks for implementing this! So for docker image deploys I would have to set SOURCE_VERSION when building the image, right? |
|
Yep! |
|
Wait no. If you're using Dockerfile or buildpack deploys the environment variable will be injected into your environment. |
|
@bacongobbler Docker image != Dockerfile =) So with "docker image deploys" I was talking about |
|
Okay then yes if you're using |
closes #1057
rebased off of #1078 for the _build_env_vars tests, see e88c52f for the new commit.