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

Add cli option to supply path to the construct yaml file #728

Closed
wants to merge 8 commits into from

Conversation

millsks
Copy link
Contributor

@millsks millsks commented Oct 25, 2023

Description

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@conda-bot
Copy link
Contributor

We require contributors to sign our Contributor License Agreement and we don't have one on file for @millsks.

In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature, merge the PR (conda/infrastructure#840), and ping the bot to refresh the PR.

Copy link
Contributor

@jaimergp jaimergp left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I am inclined to fix the reported issue but we need to do it in a non-disruptive way.

I'd rather have the flag allow to change the default construct.yaml assumption. We probably need to add some checks to ensure that it's just name and not a rel/abs path (e.g. no slashed in the value), so the join later allows to respect dir_path.

This way a user that wants a custom filename would run:

$ constructor --construct-yaml-fn=another.yaml input_directory/

which would result in constructor loading input_directory/another.yaml.

constructor/main.py Outdated Show resolved Hide resolved
constructor/main.py Outdated Show resolved Hide resolved
constructor/main.py Outdated Show resolved Hide resolved
constructor/main.py Outdated Show resolved Hide resolved
constructor/main.py Outdated Show resolved Hide resolved
constructor/main.py Outdated Show resolved Hide resolved
@millsks
Copy link
Contributor Author

millsks commented Nov 1, 2023

Thanks for your contribution! I am inclined to fix the reported issue but we need to do it in a non-disruptive way.

I'd rather have the flag allow to change the default construct.yaml assumption. We probably need to add some checks to ensure that it's just name and not a rel/abs path (e.g. no slashed in the value), so the join later allows to respect dir_path.

This way a user that wants a custom filename would run:

$ constructor --construct-yaml-fn=another.yaml input_directory/

which would result in constructor loading input_directory/another.yaml.

Thanks for the input. I greatly appreciate it. I made the changes that you suggested and ready for review.

@jaimergp
Copy link
Contributor

jaimergp commented Nov 1, 2023

Nice! Thanks. We should also add a test. Maybe modify one of the examples to have a non construct.yaml input and specify in the corresponding test_ function in tests/test_examples.py.

@millsks
Copy link
Contributor Author

millsks commented Nov 1, 2023

Thanks. I will get that in there when I get online tonight.

@jaimergp
Copy link
Contributor

jaimergp commented Jan 2, 2024

I have some commits ready to push but for some reason I get a permission denied from your fork. This is the patch:

diff --git a/constructor/main.py b/constructor/main.py
index c2ce139..7bc4998 100644
--- a/constructor/main.py
+++ b/constructor/main.py
@@ -309,8 +309,7 @@ def main():
                    action="store",
                    metavar="FILENAME",
                    dest="construct_yaml_filename",
-                   default="construct.yaml",
-)
+                   default="construct.yaml")
 
     p.add_argument('dir_path',
                    help="directory containing construct.yaml",
@@ -320,7 +319,6 @@ def main():
                    metavar='DIRECTORY')
 
     args = p.parse_args()
-
     logger.info("Got the following cli arguments: '%s'", args)
 
     if args.verbose or args.debug:
@@ -361,7 +359,7 @@ downloaded from https://repo.anaconda.com/pkgs/misc/conda-execs/""".lstrip())
     main_build(dir_path, output_dir=out_dir, platform=args.platform,
                verbose=args.verbose, cache_dir=args.cache_dir,
                dry_run=args.dry_run, conda_exe=conda_exe,
-               construct_yaml_path=args.construct_yaml_path
+               construct_yaml_filename=args.construct_yaml_path)
 
 
 if __name__ == '__main__':
diff --git a/examples/noconda/construct.yaml b/examples/noconda/constructor_input.yaml
similarity index 100%
rename from examples/noconda/construct.yaml
rename to examples/noconda/constructor_input.yaml
diff --git a/tests/test_examples.py b/tests/test_examples.py
index 55f3b27..9ae5d35 100644
--- a/tests/test_examples.py
+++ b/tests/test_examples.py
@@ -234,6 +234,7 @@ def create_installer(
     debug=CONSTRUCTOR_DEBUG,
     with_spaces=False,
     timeout=420,
+    construct_yaml_filename="construct.yaml",
     **env_vars,
 ) -> Tuple[Path, Path]:
     if sys.platform.startswith("win") and conda_exe and _is_micromamba(conda_exe):
@@ -248,6 +249,8 @@ def create_installer(
         str(input_dir),
         "--output-dir",
         str(output_dir),
+        "--construct-yaml-fn",
+        construct_yaml_filename,
     ]
     if conda_exe:
         cmd.extend(["--conda-exe", conda_exe])
@@ -346,7 +349,9 @@ def test_example_miniforge(tmp_path, request):
 
 def test_example_noconda(tmp_path, request):
     input_path = _example_path("noconda")
-    for installer, install_dir in create_installer(input_path, tmp_path, with_spaces=True):
+    for installer, install_dir in create_installer(
+        input_path, tmp_path, construct_yaml_filename="constructor_input.yaml", with_spaces=True
+    ):
         _run_installer(input_path, installer, install_dir, request=request)
 

@jaimergp
Copy link
Contributor

Superseded by #758

@jaimergp jaimergp closed this Feb 27, 2024
jezdez pushed a commit that referenced this pull request Mar 5, 2024
…728) (#758)

Co-authored-by: jaimergp <[email protected]>
Co-authored-by: Kevin Mills <[email protected]>
Co-authored-by: Kevin Mills <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

A cli option to specify the path to the construct yaml
3 participants