Add instance group support to NGINX container builder

nginxinc / docker-nginx-controller

Docker support for NGINX Controller Agent in Containers

Apache License 2.0

29 stars 26 forks source link

Add instance group support to NGINX container builder #52

Closed kushal1093 closed 3 years ago

kushal1093 commented 3 years ago

Description:

Add ENV_CONTROLLER_INSTANCE_GROUP variable to support instance group.

Reviewers:

@brianehlert @1996sajal @framer777

brianehlert commented 3 years ago

This should be an 'instance_group' just like the API and GUI - not truncated to 'group' That will make it easier to drop the 'controller' portion of the env var at some point in the future.

framer777 commented 3 years ago

LGTM, please update PR description to reflect the latest var name

@1996sajal please test and merge

1996sajal commented 3 years ago

@kushal1093, there should be readme changes for the same flag which is added here. But I don't see this MR or any other having those.

Ccing:- @brianehlert @framer777

1996sajal commented 3 years ago

kushal1093 commented 3 years ago

@kushal1093, there should be readme changes for the same flag which is added here. But I don't see this MR or any other having those.

Ccing:- @brianehlert @framer777

As for documenting the variable in the README, the variable is of no use unless DPM is merged to Master on the controller. Maybe we can document it once it is merged?

Thoughts? @brianehlert @framer777

richbrowne commented 3 years ago

@kushal1093 Please update the pull request to rename from CONTROLLER_GROUP to CONTROLLER_INSTANCE_GROUP. Let's align with instance name. Otherwise, we might confuse people who think about cgroups and containerization.

kushal1093 commented 3 years ago

@kushal1093 Please update the pull request to rename from CONTROLLER_GROUP to CONTROLLER_INSTANCE_GROUP. Let's align with instance name. Otherwise, we might confuse people who think about cgroups and containerization.

Yes, it is done. The env variable name is ENV_CONTROLLER_INSTANCE_GROUP.