Saving the VCS Checkout dir path to an environment variable #564
Conversation
| @@ -82,6 +82,7 @@ def common_setup(config, task, taskdesc, command): | |||
| for repo_config in repo_configs.values(): | |||
| checkout_path = path.join(vcs_path, repo_config.path) | |||
| command.append(f"--{repo_config.prefix}-checkout={checkout_path}") | |||
| taskdesc["worker"]["env"]["VCS_CHECKOUT_DIR_PATH"] = checkout_path | |||
There was a problem hiding this comment.
Thanks for the patch! This is close, but there are a couple things to fix:
- Can we call the env
PRIMARY_REPO_PATHinstead? - If there are multiple entries in
repo_configs, later entries will overwrite the earlier ones. Instead, I think we should only save the first entry in therepo_configs. So we should only save the env if it doesn't exist yet. - It looks like this causes the tests to fail. You'll need to fix that up before this can land. This should be a matter of updating the expectations, though it looks like the tests also set
repo_config.pathto an empty string.. it would be nice to change them to use a proper value here as well.
Thanks for your interest and please don't hesitate to reach out if you need help with any of this.
There was a problem hiding this comment.
Thanks for your message, I really appreciate the feedback.
- Changed the path var name.
- Added a check to see if a checkout dir path already exists, it will only save if the dir path does not exist.
- I am a bit unsure of how to change the
repo_config.pathto some meaningful value. I looked at the failing tests and it seems like it's primarily because the task is unable to run. So, multiple tests fail for the same reason it seems.
I would love to make any further changes if required. Thanks again for your feedback.
| try: | ||
| primary_repo_path = taskdesc["worker"]["env"]["PRIMARY_REPO_PATH"] | ||
| except Exception: | ||
| primary_repo_path_exists = False |
There was a problem hiding this comment.
This will result in an exception because primary_repo_path_exists isn't defined if we don't get to this except clause. That said, you can remove this whole block, see my comment below.
| for repo_config in repo_configs.values(): | ||
| checkout_path = path.join(vcs_path, repo_config.path) | ||
| command.append(f"--{repo_config.prefix}-checkout={checkout_path}") | ||
| primary_repo_path = taskdesc["worker"]["env"]["PRIMARY_REPO_PATH"] | ||
| if primary_repo_path is None or not vcs_path.strip(): | ||
| if primary_repo_path_exists is False or not primary_repo_path.strip(): | ||
| taskdesc["worker"]["env"]["PRIMARY_REPO_PATH"] = checkout_path |
There was a problem hiding this comment.
There's no need to check if the env was set above, Python dicts have a setdefault function that will only set the value if it doesn't already exist. E.g:
taskdesc["worker"]["env"].setdefault("PRIMARY_REPO_PATH", checkout_path)
| for repo_config in repo_configs.values(): | ||
| checkout_path = path.join(vcs_path, repo_config.path) | ||
| command.append(f"--{repo_config.prefix}-checkout={checkout_path}") | ||
| primary_repo_path = taskdesc["worker"]["env"]["PRIMARY_REPO_PATH"] | ||
| if primary_repo_path is None or not vcs_path.strip(): | ||
| if primary_repo_path_exists is False or not primary_repo_path.strip(): | ||
| taskdesc["worker"]["env"]["PRIMARY_REPO_PATH"] = checkout_path |
There was a problem hiding this comment.
This code is being executed for every loop in the for statement. You'll need to use the enumerate built-in function in the for loop to make sure you're only setting this variable on the first iteration. Otherwise, whatever is last in the list will overwrite whatever is earlier in the list.
Hi, This is my first pull request for a
good first issue#9. Please do let me know if the env path where I am saving the checkout-dir path is incorrect. I would love any feedback on my pull request.