Skip to content
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

allow exposing more environment variables #292

Merged
merged 2 commits into from
Mar 14, 2017
Merged

Conversation

mikz
Copy link
Contributor

@mikz mikz commented Mar 13, 2017

closes #284

  • expose all variables starting with APICAST_ or THREESCALE_
  • allow customizing main section of nginx via main.d

@octobot octobot added the T-obux label Mar 13, 2017
@mikz mikz force-pushed the allow-customizing-env-vars branch from edc518b to 69ba28d Compare March 13, 2017 07:42
@mikz mikz requested review from mayorova and mpguerra March 13, 2017 07:45
@mikz mikz force-pushed the allow-customizing-env-vars branch from 69ba28d to ad487bf Compare March 13, 2017 07:47
mikz added a commit that referenced this pull request Mar 13, 2017
by setting `APICAST_SERVICE_${ID}_CONFIGURATION_VERSION` environment
variable to some version it is going to be used to lock to that version

depends on #292
@mikz mikz added this to the On-premise ER5 release milestone Mar 13, 2017
@@ -110,6 +110,7 @@ done

main+=("daemon ${daemon};")
main+=("worker_processes ${worker_processes};")
main+=$(printenv | awk '$1 ~ /^(APICAST|THREESCALE)_/ {split($0,env,"="); print "env", env[1] ";"}')
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I don't like this approach too much, for the following reasons:

  1. It makes bin/apicast the only way to start APIcast that would work natively out-of-the-box. Even though it is the preferred way to start APIcast, it might not suite some users who have some already established deployment pipelines (Chef, Puppet etc, where OpenResty is started as a service). Well, yes, you can of course add all the required env definitions in main.d, but it's easy to forget something there.

  2. This way I think we are loosing the "single source of truth". It's actually super practical to have all the supported env vars listed in one place. We do have .env and parameters doc that can help, but as you usually (rightfully) say @mikz - the docs can get out of sync (the .env already did, for example) :)
    So, it would actually be good to keep the complete list in some file, that can be extended with other files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. bin/apicast is the only way to start apicast. It was before anyway. It sets up daemon mode, worker processes and everything. It finds the correct path etc etc etc. In the future we might have to template the whole nginx config, so you can forget there is nginx now.

  2. It is error prone to add everything into the configuration. Tests can be passing but without full integration suite there is no way to actually verify it. There never was single source of truth because code evaluated in the init phase had access to all the variables (even not defined) where code executed later would get them stripped. That was super confusing and not predictable.

  3. I don't see a way to get dynamic variables working for [configuration] allow locking service configuration version #293. Unless we figure out some super complicated format like: id:version;id2:version2.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Hm, OK, I thought it was just a handy way to override NGINX default values. So, as it's not the case, I think it might complicate the transition to v2/v3 for the existing customers using self-managed deployment. Ideally, APIcast would work if you just drop the contents of the repo to the default nginx conf directory and restart the process.
    But if we can't have it, I guess we'd need some kind of transition guide. @3scale/support what do you think?

  2. My point is that I as a user want to have a complete list of the env vars that APIcast "understands", so I know what to export. If it's not in the code (configuration in this case) itself, it's OK, but we just need to make sure that the doc is always in sync.

  3. OK, I was not aware of this requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mayorova want to create some checkboxes todo in the description what needs to be done for this to be merged?

  1. I don't see anywhere in the documentation that we would suggest using nginx to start by pointing to a configuration. Not sure why anyone would do that. Also not sure how many people are using v2 already (but close to 0).

  2. We can't make sure it stays in sync unless we are able to code it. Reviews fail and docs get out of sync. Proposals welcome. Tests are a good way to make sure the documentation works (and become the documentation).

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. OK, not related to this PR, probably we can discuss with @3scale/support and open a separate issue if needed.

  2. Agreed. The proposal for now would just to be disciplined and document the env vars in the parameters doc as soon as they are added/changed/deleted (which from what I can tell has been done properly lately).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory we could write a ruby script that goes through the codebase, finds strings that are in all caps and compares them to the parameters doc. This could be executed as a lint.

Maybe when we have more time for tooling and we burn ourselves .

to add env variables for example, closes #284
all variables starting with APICAST_ or THREESCALE_
will be forwarded to the nginx processes
@mikz mikz force-pushed the allow-customizing-env-vars branch from ad487bf to 203b5b9 Compare March 14, 2017 11:24
@mikz mikz merged commit 890f69f into master Mar 14, 2017
@mikz mikz deleted the allow-customizing-env-vars branch March 14, 2017 11:32
@mikz mikz removed the in progress label Mar 14, 2017
mikz added a commit that referenced this pull request Mar 14, 2017
by setting `APICAST_SERVICE_${ID}_CONFIGURATION_VERSION` environment
variable to some version it is going to be used to lock to that version

depends on #292
mikz added a commit that referenced this pull request Mar 14, 2017
by setting `APICAST_SERVICE_${ID}_CONFIGURATION_VERSION` environment
variable to some version it is going to be used to lock to that version

depends on #292
mikz added a commit that referenced this pull request Mar 14, 2017
by setting `APICAST_SERVICE_${ID}_CONFIGURATION_VERSION` environment
variable to some version it is going to be used to lock to that version

depends on #292
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support adding more env vars for customizations
3 participants