Deploy bug fixes and clean up #532
Conversation
chouseknecht
force-pushed
the
develop
branch
2 times, most recently
from
6a79a32
to
dcf64df
Compare
chouseknecht
force-pushed
the
develop
branch
3 times, most recently
from
6f8e1d4
to
ae419b4
Compare
chouseknecht
changed the title
container/config.py
Outdated
| def image_namespace(self): | ||
| # When pushing images or deploying, we need to know the registry namespace | ||
| namespace = self.project_name | ||
| if self.engine_name in ('k8s', 'openshift'): |
There was a problem hiding this comment.
I feel like we shouldn't be referencing exceptional values for specific engines outside of the engine implementation. If this needs to be a feature of an engine implementation, let's do that. But I don't want to have special engine-specific overrides outside of the engine itself any more than we have to.
There was a problem hiding this comment.
I need to know what config thinks the namespace is, so seemed logical to add a single method to the config object. There is prior art within config referencing the engine_name value, so not sure why it's suddenly an issue here.
container/config.py
Outdated
| @@ -106,7 +116,7 @@ def set_env(self, env): | |||
| config['volumes'][vol_key] = docker_settings | |||
| elif self.engine_name in ('openshift', 'k8s'): | |||
| # For openshift/k8s remove unrelated attributes | |||
| for vol_key in config['volumes'].keys(): | |||
| for vol_key in list(config['volumes'].keys()): | |||
There was a problem hiding this comment.
Isn't the py3 way of doing this: for vol_key in config['volumes']:? Why the cast?
There was a problem hiding this comment.
No idea. I'm so tired of Python 2 vs 3.
container/core.py
Outdated
| @@ -46,11 +46,12 @@ | |||
|
|
|||
| @host_only | |||
| def hostcmd_init(base_path, project=None, **kwargs): | |||
| force = kwargs.pop('force') | |||
There was a problem hiding this comment.
Maybe add to explicit args and set a default?
container/cli.py
Outdated
| @@ -264,14 +260,19 @@ def __call__(self): | |||
| LOGGING['loggers']['container']['level'] = 'DEBUG' | |||
| config.dictConfig(LOGGING) | |||
|
|
|||
| vargs = vars(args) | |||
| if not vargs['project_name']: | |||
There was a problem hiding this comment.
Outside of init, wouldn't this make more sense to be part of the container.yml settings? So it doesn't have to be specified every single time the user invokes ansible-container?
There was a problem hiding this comment.
We could add project_name to settings, yes. However, that's not the point of this change. The point here is to reduce redundant code in core.py.
container/core.py
Outdated
| 'host_user_uid': os.getuid(), | ||
| 'host_user_gid': os.getgid(), | ||
| } | ||
| if config.get('settings', {}).get('k8s_auth'): | ||
| params['k8s_auth'] = config['settings']['k8s_auth'] | ||
| if config.get('settings', {}).get('k8s_namespace'): |
There was a problem hiding this comment.
Reiterated comment about trying best we can to keep engine-specific snowflakes in the engine specification.
container/docker/engine.py
Outdated
| output_path = params['deployment_output_path'] | ||
| for path in ('files', 'templates'): | ||
| shutil.rmtree(os.path.join(output_path, path), ignore_errors=True) | ||
| if not self.debug: |
There was a problem hiding this comment.
Maybe this should be devel instead of debug? I'm not sure either way.
container/k8s/base_engine.py
Outdated
| if service_config.get('roles'): | ||
| if url and namespace: | ||
| # Reference previously pushed image | ||
| self.services[service_name][u'image'] = '{}/{}/{}'.format(re.sub(r'/$', '', url), namespace, |
There was a problem hiding this comment.
You can do this without re - kinda heavy handed. Just do url.rstrip('/')
test/tests/validate_config.py
Outdated
| @@ -9,7 +9,7 @@ | |||
| import container | |||
| from container.config import AnsibleContainerConfig, AnsibleContainerConductorConfig | |||
| from container.exceptions import AnsibleContainerConfigException | |||
| from ansible.vars import Templar | |||
| from ansible.template import Templar | |||
There was a problem hiding this comment.
QTNA: Do we want to try/except these imports?
test/tests/validate_build.py
Outdated
| @@ -8,7 +8,7 @@ | |||
| import os | |||
| import json | |||
|
|
|||
| from ansible.vars import Templar | |||
| from ansible.template import Templar | |||
There was a problem hiding this comment.
QTNA: Do we want to try/except these imports?
chouseknecht
force-pushed
the
develop
branch
2 times, most recently
from
fde348a
to
1af8c1f
Compare
ISSUE TYPE
SUMMARY
ansible/django-gulp-nginx, and fixing bugs and updating docs as a result.