Skatteetaten / terraform-nomad-postgres

Apache License 2.0
8 stars 5 forks source link

Switch for gitlab images, and Gihub workflow skip-linter label #60

Closed claesgill closed 3 years ago

claesgill commented 3 years ago

Closes/fixes/resolves issue(s)?

Closes #52

What was added/changed/fixed?

Related issue(s)? [Optional]

Others [Optional]

Checklist (after created PR)

zhenik commented 3 years ago

Why have separate switch? We could implement image variables with default values for docker hub. If needed, these can be changed to whatever image.-.

I agree.

We could also setup default values pointing to default for sidecar_task.config.image pointing to meta.connect.sidecar_image or envoyproxy/envoy:v${NOMAD_envoy_version} from documentation

claesgill commented 3 years ago

Why have separate switch? We could implement image variables with default values for docker hub. If needed, these can be changed to whatever image.-.

That's a fairly good point. However, I think the switch makes it easier to quickly debug/test locally.

Because our github workflow requires to pull images from Docker Hub we would need the default images to be linked to Docker. Without the switch we would need to type out the GitLab address (e.g. gitlab-container-registry.service.v2.minerva.loc/datastack/terraform-nomad-postgres/postgres:12-alpine) each time we work on a new branch..

If we can find a way of avoiding to type or copy/paste the custom image address, I agree that we can remove the switch.

pdmthorsrud commented 3 years ago

After looking at it I think variables with default might be more suitable. It's easier to read, and makes a lot of sense; you "supply" your own image, or else you use "our" (default) images

claesgill commented 3 years ago

@pdmthorsrud @dangernil @zhenik @fredrikhgrelland I don't think this is as relevant anymore. Do you agree that we close this PR?