-
Notifications
You must be signed in to change notification settings - Fork 21
Adding an optional version env var for the version endpoint to return #121
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
Conversation
5671f51
to
e318072
Compare
:rtype: dict or None | ||
""" | ||
version_json = os.path.join(root, "version.json") | ||
version_env_var = os.getenv("DOCKERFLOW_VERSION") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add this additional source of information instead of using the version.json
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We adjusted the remote-settings release process to re-tag the existing image instead of building a fresh one (link). This reduces our release times and allows us to use the exact same image that's been working great already in dev/stage.
But, the version file doesn't then get rebuilt so our version endpoint is incorrect. My best ideas to solve this were:
- Allow for an optional version parameter to be sent (this PR basically)
- Update and mount a configmap for the
version.json
file on deploy
This felt a little easier to me but I'm open to option 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think sliding the version in outside of the version.json
file doesn't make sense and breaks the contract of version.json
.
I think it'd be better to have a short Dockerfile
template, expand the template with the image details of the stage image you're using, and then COPY the updated version.json
file.
You should talk with @grahamalama and see if that's something we can put in deploy-actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Yeah that's another option and is pretty simple to implement
tests/core/test_version.py
Outdated
def test_env_var_override(tmpdir): | ||
content = {"spam": "eggs"} | ||
mock.patch.dict(os.environ, { "DOCKERFLOW_VERSION": "foo"}) | ||
version_json = tmpdir.join("version.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This library uses pytest, so you should use its monkeypatching:
https://docs.pytest.org/en/stable/how-to/monkeypatch.html
def test_env_var_override(tmpdir): | |
content = {"spam": "eggs"} | |
mock.patch.dict(os.environ, { "DOCKERFLOW_VERSION": "foo"}) | |
version_json = tmpdir.join("version.json") | |
def test_env_var_override(tmpdir, monkeypatch): | |
content = {"spam": "eggs"} | |
monkeypatch.setenv("DOCKERFLOW_VERSION", "foo") | |
version_json = tmpdir.join("version.json") |
e318072
to
46ee69b
Compare
No description provided.