Base image can now be specified in the definition file #129
Conversation
beeankha
force-pushed
the
base_image_in_def_file
branch
from
74483ea
to
e52c5a5
Compare
|
recheck |
ansible_builder/main.py
Outdated
| @@ -190,6 +198,15 @@ def validate(self): | |||
| if not os.path.exists(requirement_path): | |||
| raise DefinitionError("Dependency file {0} does not exist.".format(requirement_path)) | |||
|
|
|||
| ee_base_image = self.get_base_image() | |||
There was a problem hiding this comment.
I hate to see get_base_image as its own method when it's not the canonical way that the value gets set in practice. I'd prefer to lose the method and use the same self.definition.raw.get('base_image') here
ansible_builder/main.py
Outdated
| @@ -32,12 +32,13 @@ def __init__(self, action=None, | |||
| verbosity=0): | |||
| self.action = action | |||
| self.definition = UserDefinition(filename=filename) | |||
| self.base_image = self.definition.raw.get('base_image') or base_image | |||
There was a problem hiding this comment.
I don't know about the precedence. If I provide the base image both in the definition file, and with the CLI option... I would think the CLI option should win. That tends to be how we want config processing to go. @shanemcd what do you think? Or else we could error if given both.
We would have to re-think how the kwarg default works, it might make more sense for None to be the value from the kwarg.
There was a problem hiding this comment.
If I provide the base image both in the definition file, and with the CLI option... I would think the CLI option should win.
Agreed, I'll switch those around and clarify the precedence in the docs.
There was a problem hiding this comment.
Have made some updates, will work on making sure integration tests pass next.
beeankha
force-pushed
the
base_image_in_def_file
branch
from
2578b6c
to
5ead3ef
Compare
test/test_main.py
Outdated
| @@ -109,25 +105,29 @@ def test_definition_syntax_error(self, data_dir): | |||
| path = os.path.join(data_dir, 'definition_files/bad.yml') | |||
|
|
|||
| with pytest.raises(DefinitionError) as error: | |||
| AnsibleBuilder(filename=path) | |||
| AnsibleBuilder(filename=path, base_image='my-custom-image') | |||
There was a problem hiding this comment.
these test changes shouldn't be needed, should they? I'd like to see if we can roll some of those back.
ansible_builder/main.py
Outdated
| raise DefinitionError(textwrap.dedent( | ||
| """ | ||
| Error: Please specify a base image for the execution environment. | ||
| """) |
There was a problem hiding this comment.
I don't know about this, I would prefer to keep the default behavior the same.
|
put up revised suggestions in beeankha#2 |
Review suggestions for base image default handling
beeankha
force-pushed
the
base_image_in_def_file
branch
from
59e4700
to
8467ce7
Compare
|
recheck |
Addressing Issue #119