hackoregon / backend-examplar-2018

an example dockerized geo-aware django backend
MIT License
4 stars 5 forks source link

add $PRODDOCKER_IMAGE and tag docker push with travis build number #84

Closed nam20485 closed 6 years ago

nam20485 commented 6 years ago

Adds $PRODUCTION_DOCKER_IMAGE and $PRODUCTION_DOCKER_IMAGE and sets them to "backend-examplar-service".

Tags registry's docker image with "branch-travis build number" in addition to "latest"

MikeTheCanuck commented 6 years ago

In hopes of later being able to assemble a full accounting of all the environment variables in use throughout the API projects (so that we know how to support and properly configure them), I'd like us to document these env vars clearly to purpose and where they're being generated and consumed. They seem self-evident in this PR, but give it a few weeks and they'll just get caught up in the ever-expanding family of env vars with similar names.

MikeTheCanuck commented 6 years ago

I'd also like to see documentation (at least here, if not in some form of README-tagging.md or similar) that explains why we're tagging, what each tag will be used for, and examples to make it clear what we'll get from the docker tag commands. (Maybe I'm the only one, but at this point I can't construct a mental model of what we're getting with these additions, and I'd hate to try to reconstruct it next year with no docs.)

MikeTheCanuck commented 6 years ago

As far as I can tell, these are the new env vars being introduced as part of this PR:

Are any of these intended to fully replace any legacy or new env vars, or stand in addition to?

nam20485 commented 6 years ago
nam20485 commented 6 years ago

You can see a full accounting of the required variables for this scheme on https://travis-ci.org/hackoregon/backend-examplar-2018. I also updated the sample.env to include the new ones.

...
Setting environment variables from repository settings
$ export AWS_ACCESS_KEY_ID=[secure]
$ export AWS_DEFAULT_REGION=us-west-2
$ export AWS_SECRET_ACCESS_KEY=[secure]
$ export DOCKER_REPO=845828040396.dkr.ecr.us-west-2.amazonaws.com
$ export DOCKER_REPO_NAMESPACE=production
$ export PRODUCTION_DOCKER_IMAGE=backend-exemplar-service
$ export PRODUCTION_DOCKER_SERVICE=backend-exemplar-service
...
nam20485 commented 6 years ago

If you look in the ECR where I am docker pushing to you will see a visual representation of what I am trying to convey that will make sense. Before, after multiple pushes, the ECR would contain one image with the "latest" tag and a bunch of others with all no/blank tag. Now, you would see one with a "latest" tag AND something like "development-74", as well as several more with only "development-73," "develpoment-74," "development-72", ... The new tag catalogs which branch and travis build number the image was built and pushed during. In this way, we can track which source code changes are in each image.

MikeTheCanuck commented 6 years ago

OK - if I understand your messages and the state of this PR, I'll wait for additional updates before being able to fully review this.

Question about tagging intent: when we look in ECR, is your intent that by seeing the "development-74" tag, we will be able to trace that image back to...hmmm. Knowing the travis build could be helpful, but I'm also wondering how well any of us will remember that "development-74" refers to Travis build and not GitHub PR #. Is it possible to add a static string to a tag, such as "Travis-build-74", to remove the ambiguity/make the info more self-evident?

Which raises another question for me - whereas some projects are trying stable branches, others use ephemeral branches per-PR, and in the latter cases I'm not sure that I can see the value in knowing the name of the originating branch for the merge. (more I think about it, I'm not even sure it's helpful to know which long-lived branch originated the new image)

nam20485 commented 6 years ago

Yep, I'll make changes to address the requests you have made and then push another commit.

I agree, I'll remove the branch (its contained in the Travis build log anyhow) and add a static tag so it looks like "travis-buildnum-nn".

On Sat, May 12, 2018, 5:04 PM Mike Lonergan notifications@github.com wrote:

OK - if I understand your messages and the state of this PR, I'll wait for additional updates before being able to fully review this.

Question about tagging intent: when we look in ECR, is your intent that by seeing the "development-74" tag, we will be able to trace that image back to...hmmm. Knowing the travis build could be helpful, but I'm also wondering how well any of us will remember that "development-74" refers to Travis build and not GitHub PR #. Is it possible to add a static string to a tag, such as "Travis-build-74", to remove the ambiguity/make the info more self-evident?

Which raises another question for me - whereas some projects are trying stable branches, others use ephemeral branches per-PR, and in the latter cases I'm not sure that I can see the value in knowing the name of the originating branch for the merge. (more I think about it, I'm not even sure it's helpful to know which long-lived branch originated the new image)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hackoregon/backend-examplar-2018/pull/84#issuecomment-388591601, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvqFErdEXSsaMCU4F41SKjBMhPtn_j0ks5tx3iHgaJpZM4T8Oek .

nam20485 commented 6 years ago

As far as I can tell, these are the new env vars being introduced as part of this PR:

DOCKER_REPO_NAMESPACE PRODUCTION_DOCKER_IMAGE DOCKER_PATH TAG PRODUCTION_DOCKER_SERVICE Are any of these intended to fully replace any legacy or new env vars, or stand in addition to?

DOCKER_REPO_NAMESPACE replaces DEPLOY_TARGET PRODUCTION_DOCKER_IMAGE replaces DOCKER_IMAGE PRODUCTION_DOCKER_SERVICE replaces DOCKER_SERVICE

MikeTheCanuck commented 6 years ago

Have you had a chance to make the adjustments you referenced here here Nathan? I assumed they would be fixed before we considered this done.

MikeTheCanuck commented 6 years ago

I'm also remembering a conversation with @BrianHGrant from a week or two back about removing "PRODUCUTION_" prefixes from a bunch of env vars, and I had assumed (hoped?) that we would be doing the same for the PRODUCTION_DOCKER_IMAGE and PRODUCTION_DOCKER_SERVICE env vars here, to push them towards DOCKER_IMAGE and DOCKER_SERVICE naming instead.

nam20485 commented 6 years ago

I'm also remembering a conversation with @BrianHGrant from a week or two back about removing "PRODUCUTION_" prefixes from a bunch of env vars, and I had assumed (hoped?) that we would be doing the same for the PRODUCTION_DOCKER_IMAGE and PRODUCTION_DOCKER_SERVICE env vars here, to push them towards DOCKER_IMAGE and DOCKER_SERVICE naming instead.

I believe it is in this PR: https://github.com/hackoregon/backend-examplar-2018/pull/88

nam20485 commented 6 years ago

Have you had a chance to make the adjustments you referenced here here Nathan? I assumed they would be fixed before we considered this done.

Yes, I addressed each of the issues you raised, and then re-committed. I think it was changing the docker image tag to remove the branch, and use $REMOTE_DOCKER_PATH in one more placpe where I had missed it.

iant01 commented 6 years ago

With adding tags to the ECR images, the repo lifecycle rule(s) will need to be adjusted to account for image other than "latest" having tags. Right now only untagged images are pruned to most recent 10 images. If different projects use different tags, each repo will need a custom lifecycle rule to manage the number of retained images.

MikeTheCanuck commented 6 years ago

Um, I feel like an a** ... but after we've reverted the PRODUCTION_ prefixes via PR #88, I'm having a hard time remembering why it's advantageous to add the same prefix to DOCKER_IMAGE and DOCKER_SERVICE via this PR. Lacking memory of the history, it doesn't seem to add clarity (and maybe it was only done to be consistent with the pre-PR #88 pattern?).

Is there any chance you'd be willing to go back to the pre-prefixed versions of these env vars?

nam20485 commented 6 years ago

I saw Brian's PR for removing the PRODUCTON_ prefixes, but I must have missed the merge and revert. So now the plan is for leaving the PRODUCTION_ prefixes in?

If so, that is no problem, I can change $PRODUCTION_DOCKER_SERVICE variable back to `$DOCKER_SERVICE".

However, I do not think it makes sense to also change the $PRODUCTION_DOCKER_IMAGE variable to $DOCKER_IMAGE. This is used by the development image's docker config and scripts in bin/, much like the $PRODUCTION_DOCKERIMAGE is used in the production docker image's config and scripts. In other words this variable needs to be split into two versions like the other PRODUCTION and DEVELOPMENT_ variables do, so that the two different sets of image configs (development and production) continue using their own copy of the setting. If we do un-split it back to just one $DOCKER_IMAGE variable, the developments image's docker config/script files will need to be updated also, or will not function as they are intended. They may not work anymore, or if they do, then they won't be buildable/runnable on their own independent of the production images, which is how the whole system was designed.

MikeTheCanuck commented 6 years ago

Yipes, sorry - I miscommunicated. We've taken Brian's PR and renamed most env vars back to the simpler versions e.g. PRODUCTION_POSTGRES_USER is now POSTGRES_USER. Does that help?

nam20485 commented 6 years ago

Yes that helps. I will rename $PRODUCTION_DOCKER_SERVICE and $PRODUCTION_DOCKER_IMAGE to $DOCKER_SERVICE and $DOCKER_IMAGE.

nam20485 commented 6 years ago

Yes that helps. I will rename $PRODUCTION_DOCKER_SERVICE and $PRODUCTION_DOCKER_IMAGE to $DOCKER_SERVICE and $DOCKER_IMAGE.

Completed and committed.

MikeTheCanuck commented 6 years ago

This is solid now, this is the pattern we're using in live API repos.