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

Implement singularity-compose.yml override configuration feature #48

Merged
merged 27 commits into from
Oct 7, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
5f7bb40
implement deep_merge functionally
PauloMigAlmeida Oct 5, 2021
8c7a592
fix typo
PauloMigAlmeida Oct 5, 2021
5491961
implement some changes suggested during code review
PauloMigAlmeida Oct 5, 2021
2285aba
add tests for the config override feature
PauloMigAlmeida Oct 6, 2021
e2fbef0
good old black formatting that I keep forgetting :)
PauloMigAlmeida Oct 6, 2021
7deee7d
add docs
PauloMigAlmeida Oct 6, 2021
746fb07
adding support for singularity-compose check
vsoch Oct 6, 2021
41731fa
implement deep_merge functionally
PauloMigAlmeida Oct 5, 2021
e7e6694
fix typo
PauloMigAlmeida Oct 5, 2021
e1f5bf6
implement some changes suggested during code review
PauloMigAlmeida Oct 5, 2021
14996ab
add tests for the config override feature
PauloMigAlmeida Oct 6, 2021
0fa8477
good old black formatting that I keep forgetting :)
PauloMigAlmeida Oct 6, 2021
35388e9
add docs
PauloMigAlmeida Oct 6, 2021
7a35b99
Merge remote-tracking branch 'origin/ft/override_files' into ft/overr…
PauloMigAlmeida Oct 6, 2021
6f9544f
tweak check group so it takes into account multiple files instead
PauloMigAlmeida Oct 6, 2021
01a522a
implement preview option
PauloMigAlmeida Oct 6, 2021
2d10fe3
update changelog and add docs
PauloMigAlmeida Oct 6, 2021
a40e8f4
Apply suggestions from code review
PauloMigAlmeida Oct 7, 2021
900d8ee
move deep_merge stuff to a brand new module
PauloMigAlmeida Oct 7, 2021
949b088
black formatting
PauloMigAlmeida Oct 7, 2021
86c0905
remove extra bullet point
PauloMigAlmeida Oct 7, 2021
acea351
implement code review changes
PauloMigAlmeida Oct 7, 2021
22dd789
implement code review changes
PauloMigAlmeida Oct 7, 2021
37f9e7e
bitten by the black formatting (again)
PauloMigAlmeida Oct 7, 2021
2d75868
move config functions into proper module
vsoch Oct 7, 2021
779456a
trying to pin black to uptodate version
vsoch Oct 7, 2021
352ee85
use bot.exit over print and sys.exit (which combines the two)
vsoch Oct 7, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion scompose/client/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ def get_parser():
"-f",
dest="file",
help="specify compose file (default singularity-compose.yml)",
default="singularity-compose.yml",
action="append",
default=[],
)

parser.add_argument(
Expand Down Expand Up @@ -225,6 +226,11 @@ def show_help(return_code=0):
print(scompose.__version__)
sys.exit(0)

# argparse inherits a funny behaviour that appends default values to the dest value whether you've specified a value
# or not. The bug/behaviour is documented here: https://bugs.python.org/issue16399
if len(args.file) == 0:
args.file = ["singularity-compose.yml"]

# Does the user want a shell?
if args.command == "build":
from scompose.client.build import main
Expand Down
63 changes: 52 additions & 11 deletions scompose/project/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ def set_filename(self, filename):
==========
filename: the singularity-compose.yml file to use
"""
self.filename = filename or "singularity-compose.yml"
self.working_dir = os.path.dirname(os.path.abspath(self.filename))
self.filename = filename
Copy link
Member

Choose a reason for hiding this comment

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

I would leave the default here to "singularity-compose.yml" - and if you are changing this to require a list, then please support the old behavior to still provide a single filename, and have it checked if it's a list, single string, or None, and process accordingly. And then if your preference is to store in a list, the variable should be self.filenames to indicate plural / multiple files.

And if I understand correctly, the working directory is changed to os.getcwd() to account for possibly different compose files in different directories. Of course the issue here is that all compose files need to then have their context relevant to that directory (is that your intention?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would leave the default here to "singularity-compose.yml"

The reason why I removed it is because this default value comes from the argparse as well, so I thought it was redundant. Quick question, the reason why you want the default value here as well is because you are envisioning devs instantiating this class programatically instead of using the CLI, is that correct?

Copy link
Member

Choose a reason for hiding this comment

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

yes correct! We wouldn't want to break previous scripts that are providing a string. The simple solution I think is to allow either.

self.working_dir = os.getcwd()

def set_name(self, name):
"""set the filename to read the recipe from. If not provided, defaults
Expand All @@ -75,11 +75,9 @@ def set_name(self, name):
==========
name: if a customize name is provided, use it
"""
pwd = os.path.basename(os.path.dirname(os.path.abspath(self.filename)))
self.name = (name or pwd).lower()
self.name = (name or self.working_dir).lower()

# Listing

def ps(self):
"""ps will print a table of instances, including pids and names."""
instance_names = self.get_instance_names()
Expand Down Expand Up @@ -131,7 +129,6 @@ def get_instance(self, name):
return instance

# Loading Functions

def get_already_running(self):
"""Since a user can bring select instances up and down, we need to
derive a list of already running instances to include
Expand All @@ -148,15 +145,59 @@ def get_already_running(self):
def load(self):
"""load a singularity-compose.yml recipe, and validate it."""

if not os.path.exists(self.filename):
bot.error("%s does not exist." % self.filename)
sys.exit(1)

try:
self.config = read_yaml(self.filename, quiet=True)
yaml_files = []

for f in self.filename:
# ensure file exists
if not os.path.exists(f):
bot.error("%s does not exist." % f)
Copy link
Member

Choose a reason for hiding this comment

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

Please move the multiple load of yaml files outside of the project.py - there should ideally be a separate utils function or set of config functions (some config.py or similar) that takes the list. and returns the single config here. Also - self.filename implies one file, and there should either be a self.filenames or self.filename to match the arity.

sys.exit(1)
# read yaml file
yaml_files.append(read_yaml(f, quiet=True))

# merge/override yaml properties where applicable
self.config = self.deep_merge(yaml_files)
except: # ParserError
bot.exit("Cannot parse %s, invalid yaml." % self.filename)

def deep_merge(self, yaml_files):
"""merge singularity-compose.yml files into a single dict"""
if len(yaml_files) == 1:
# nothing to merge as the user specified a single file
return yaml_files[0]

base_yaml = None
for idx, item in enumerate(yaml_files):
if idx == 0:
base_yaml = item
else:
base_yaml = self.merge(base_yaml, item)

return base_yaml

Copy link
Member

Choose a reason for hiding this comment

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

All of this logic should not be in project.py - it's for merging yaml.

def merge(self, a, b):
Copy link
Member

Choose a reason for hiding this comment

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

Move this too.

"""merge dict b into a"""
for key in b:
if key in a:
# merge dicts recursively
if isinstance(a[key], dict) and isinstance(b[key], dict):
a[key] = self.merge(a[key], b[key])

# if types are equal, b takes precedence
elif isinstance(a[key], type(b[key])):
a[key] = b[key]

# if nothing matches then this means a conflict of types which shouldn't exist in the first place
else:
bot.error(
"key %s has property type mismatch in different files." % key
)
sys.exit(1)
else:
a[key] = b[key]
return a

def parse(self):
"""parse a loaded config"""

Expand Down