Simplify git cloning #38
Conversation
This commit replaces the https url for the private packages with github ssh and changes the logic in ska_builder.py to just use whatever is in that url (instead of guessing a github ssh string from the package name).
| repo = git.Repo.clone_from(url, clone_path) | ||
| print("Cloned from url {}".format(url)) | ||
| metayml = os.path.join(pkg_defs_path, name, "meta.yaml") | ||
| # It isn't clean yaml at this point, so just extract the string we want after "home:" |
There was a problem hiding this comment.
It looks like the previous requires/path loop was dealing with the fact that the meta.yaml isn't compliant YAML at this point at the path key. I just went with a regex here to grab the string after home: instead of trying to use the YAML parser. I think that is fair for our application.
There was a problem hiding this comment.
I suspect it could be made into compliant YAML by enclosing the {{ .. }}/.. stuff in double quotes, but I agree with your point to just get the job done here.
ska_conda/ska_builder.py
Outdated
| failures.append(pkg_name) | ||
| continue | ||
| if len(failures): | ||
| raise ValueError("Packages {} failed".format(",".join(failures))) |
There was a problem hiding this comment.
This isn't strictly required, but I thought it might be helpful to let the build process continue through the rest of the packages even if you have an issue with one package (an issue such as forgetting your ssh key password) and then printing out the failures and exiting with error status at the end.
There was a problem hiding this comment.
I sort of think that if something fails, there is really a problem that needs to be fixed before moving on (and maybe you can into dependency hell where every build starts failing because of the missing failed build). Since it doesn't re-build everything every time (right?), it seems pretty cheap to restart. The "forgetting ssh key" seems a not-credible failure.
| repo = git.Repo.clone_from(url, clone_path) | ||
| print("Cloned from url {}".format(url)) | ||
| metayml = os.path.join(pkg_defs_path, name, "meta.yaml") | ||
| # It isn't clean yaml at this point, so just extract the string we want after "home:" |
There was a problem hiding this comment.
I suspect it could be made into compliant YAML by enclosing the {{ .. }}/.. stuff in double quotes, but I agree with your point to just get the job done here.
ska_conda/ska_builder.py
Outdated
| metayml = os.path.join(pkg_defs_path, name, "meta.yaml") | ||
| # It isn't clean yaml at this point, so just extract the string we want after "home:" | ||
| meta = open(metayml).read() | ||
| url = re.search("home:\s?(\S+)", meta).group(1) |
There was a problem hiding this comment.
Should be \s*. Multiple spaces are fine. I can't remember if zero spaces is OK for YAML, but for this purpose yes.
ska_conda/ska_builder.py
Outdated
| failures.append(pkg_name) | ||
| continue | ||
| if len(failures): | ||
| raise ValueError("Packages {} failed".format(",".join(failures))) |
There was a problem hiding this comment.
I sort of think that if something fails, there is really a problem that needs to be fixed before moving on (and maybe you can into dependency hell where every build starts failing because of the missing failed build). Since it doesn't re-build everything every time (right?), it seems pretty cheap to restart. The "forgetting ssh key" seems a not-credible failure.
| clone_path = os.path.join(self.src_dir, name) | ||
| if not os.path.exists(clone_path): | ||
| # Try ssh first to avoid needing passwords for the private repos |
There was a problem hiding this comment.
Regarding continuing, my thought was that there could be other reasons for failure that you might not care about if all you really wanted to do was update a couple of packages at the end of the list. Or if you were a non-privileged user who'd just been handed this code base (JWST?) and this logic lets you really just see how everything works without stopping at parse_cm. You could get in to dependency hell in a fresh build with no access to the currently build packages in ska3-conda, but the only fallout of this change in that case would just be that it would take a lot longer to finish failing out with the rest of the packages.
There was a problem hiding this comment.
Also, "forgetting ssh key password" was a creditable failure in my testing.
There was a problem hiding this comment.
OK, well then put in a text input to ask the user to continue ala install.py.
|
Is this good to go @taldcroft ? |
This PR removes the logic that tries to git clone packages using ssh by default, and instead just explicitly uses whatever string is after "home:" in the meta.yaml. For private packages, that is now set to the github ssh string. For the rest, it is set to the https url.
Closes #8 (via explicit compromise on https and ssh urls)