Skip to content
This repository has been archived by the owner on Mar 18, 2021. It is now read-only.

allowing consul service tags to be set #87

Merged
merged 1 commit into from
Jan 7, 2020
Merged

allowing consul service tags to be set #87

merged 1 commit into from
Jan 7, 2020

Conversation

attachmentgenie
Copy link
Contributor

i have a need to setu consul service tag for the openfaas functions, i implemented this similar to #65

Copy link
Collaborator

@acornies acornies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks ok, just wondering if env vars is the right place for this? (good addition btw) There are also the label and annotation collections.

@attachmentgenie
Copy link
Contributor Author

The code looks ok, just wondering if env vars is the right place for this? (good addition btw) There are also the label and annotation collections.

Forcing more and more into envars felt a bit dirty to me too, i looked into labels and annotations as well. Which take KV pairs [1] which tags can be, be mostly aren't.

Therefor i stopped my efforts into creating a pull request for implementing tags as one of those. for future reference i had selected annotations as the route to go down, although those also get used to set metadata at the job level. I choose annotations over labels since labels are implemented at driver level and annotations in the main dsl.

if you believe going down that path is more appropriate then i ll create a PR using those instead
[1] https://docs.openfaas.com/reference/yaml/#function-labels

@acornies
Copy link
Collaborator

acornies commented Jan 6, 2020

Based on The OpenFaaS docs we probably should store these using annotations. Env vars (in my mind) are usually for setting config for the function/app/microservice code.

I recognize that we've already broken this rule in the provider, so I actually think this is OK to merge and provide users a way to set consul service tags.

I have an ongoing task/repo with a focus on a rewrite of the provider using up-to-date deps. Perhaps we can address it differently there: https://github.com/acornies/faas-nomad-x

@acornies acornies merged commit 099e82c into hashicorp:master Jan 7, 2020
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.

None yet

2 participants