-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: add python as a supported build tool #67
Conversation
20cca24
to
c7c1eef
Compare
|
||
"""This module contains the Pip class which inherits BaseBuildTool. | ||
|
||
This module is used to work with repositories that use Poetry for dependency management. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module is used to work with repositories that use Poetry for dependency management. | |
This module is used to work with repositories that use pip for dependency management. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in f7bec7f
pattern = os.path.join(path, "**", file_name) | ||
files_detected = glob.glob(pattern, recursive=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if adding this wrapper function over glob.glob
makes too much sense. All it's doing is calling glob.glob()
really and I think this could just be done directly at is_detected
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I've removed this function and put the functionality inside is_detected
instead in f7bec7f
if files_detected: | ||
try: | ||
# Take the highest level file (shortest file path) | ||
file_path = min(files_detected, key=len) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the len
of the first level path part could be larger than a path with more parts, e.g., a/b/c.txt
vs aaaaaaaaaaaa/c.txt
. You would need to get the number of parts here probably:
len(Path(x).parts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I changed the key
function to look for parts instead in f7bec7f
def prepare_config_files(self, wrapper_path: str, build_dir: str) -> bool: | ||
"""Prepare the necessary wrapper files for running the build. | ||
|
||
This method will return False if there is any errors happened during operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method will return False if there is any errors happened during operation. | |
This method returns False on errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in f7bec7f
Returns | ||
------- | ||
bool | ||
True if succeed else False. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True if succeed else False. | |
True if succeeds else False. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in f7bec7f
9ad67a9
to
98eaf59
Compare
@@ -136,7 +218,7 @@ def run_check(self, ctx: AnalyzeContext, check_result: CheckResult) -> CheckResu | |||
os.path.basename(bash_cmd["CI_path"]), | |||
) | |||
|
|||
justification: list[str | dict[str, str]] = [ | |||
justification_cmd: list[str | dict[str, str]] = [ | |||
{ | |||
f"The target repository uses build tool {build_tool.name} to deploy": bash_source_link, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Python projects, rather than saying (i.e.) "...uses build tool pip to deploy", should this reflect the actual publishing tool used, to be more descriptive? Something like: "...uses build tool pip, with Twine to deploy".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Python projects, rather than saying (i.e.) "...uses build tool pip to deploy", should this reflect the actual publishing tool used, to be more descriptive? Something like: "...uses build tool pip, with Twine to deploy".
A more descriptive justification would be good, but the original message was logged with the assumption that the command would already contain the exact build tool and deploy command. Please feel free to adjust the message and push for review if you have a better message in mind.
src/macaron/config/defaults.ini
Outdated
twine | ||
flit | ||
conda | ||
builder_module = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a description for this attribute. It might not be straightforward for someone who is new to our codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9c167f2.
------- | ||
bool | ||
True if succeed else False. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a comment that pip
does not require any preparation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9c167f2.
DependencyAnalyzer | ||
The DependencyAnalyzer object. | ||
""" | ||
return NoneDependencyAnalyzer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a TODO to implement it later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9c167f2.
bool | ||
True if succeeds else False. | ||
""" | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we returning False
here? Also please add a comment if preparation is not necessary for poetry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9c167f2.
DependencyAnalyzer | ||
The DependencyAnalyzer object. | ||
""" | ||
return NoneDependencyAnalyzer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as pip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9c167f2.
@@ -116,6 +133,34 @@ def test_build_as_code_check(self) -> None: | |||
gradle_deploy.dynamic_data["ci_services"] = [ci_info] | |||
assert check.run_check(gradle_deploy, check_result) == CheckResultType.PASSED | |||
|
|||
# Use poetry publish to publish the artifact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Use poetry publish to publish the artifact | |
# Use poetry publish to publish the artifact. |
Same for comments below.
@@ -147,3 +192,28 @@ def test_build_as_code_check(self) -> None: | |||
bash_commands["commands"] = [] | |||
maven_deploy.dynamic_data["ci_services"] = [ci_info] | |||
assert check.run_check(maven_deploy, check_result) == CheckResultType.FAILED | |||
|
|||
# This Github Actions workflow uses gh-action-pypi-publish to publish the artifact. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a new function to test GitHub Actions workflow deployment? This function is getting hard to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed in 9c167f2. Refactored this file to use functions (+ fixtures) rather than the class-based approach. I added several fixtures for the build tools and CI services into conftest.py
so they can be shared across many tests.
assert check.run_check(pip_module_build_ci, check_result) == CheckResultType.FAILED | ||
|
||
# Use pip as a module in CI with invalid goal to build the artifact. | ||
pip_module_build_ci = AnalyzeContext("use_build_tool", os.path.abspath("./"), MagicMock()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you adjust the attribute names in the spec, please update the variable names here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9c167f2.
@@ -147,3 +205,5 @@ def test_build_service_check(self) -> None: | |||
ci_info["service"] = gitlab_ci | |||
maven_build_ci.dynamic_data["ci_services"] = [ci_info] | |||
assert check.run_check(maven_build_ci, check_result) == CheckResultType.FAILED | |||
|
|||
# TODO: Python module - maybe not for this context, just build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was meant to be removed, done in 9c167f2.
@sophie-bates Please update the |
cfg_path = next(list_iter) | ||
yield Path(cfg_path).parent.relative_to(repo_path) | ||
while next_item := next(list_iter): | ||
if str(Path(cfg_path).parent) in next_item: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend we very careful of the in
between two strings because the behavior might be implicit. For example, if cfg_path
is /home/boo
and next_item
is /home/foo/home/boo/
, if str(Path(cfg_path).parent) in next_item:
will be True and we will ignore the valid /home/foo/home/boo/
.
I think we could use .startswith
here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I based this abstraction off the implementation in BaseBuildTool.get_build_dirs(), so this function would need updating too if that's the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@behnazh-w How do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I based this abstraction off the implementation in BaseBuildTool.get_build_dirs(), so this function would need updating too if that's the case.
That's not possible because first we sort the elements based on the parent path. So /home/foo/home/boo/
should never be filtered by /home/boo
. But using .startswith
would make it more explicit.
Regardless, I'm not sure if it makes sense to override this function to add config_files = self.build_configs + self.package_lock
. As discussed, poetry.lock
on its own cannot determine a build tool and pyproject.toml
must always exist for poetry builds. So there shouldn't be any need to add self.package_lock
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@behnazh-w that's a good point. I had done it this way before we adjusted the config variables, and now I agree that we don't need to override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this override in 29cdd3a.
tests/conftest.py
Outdated
|
||
Parameters | ||
---------- | ||
setup_test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc is inconsistent with the parameter provided to this fixture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this fixture should be passed the setup_test
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 29cdd3a.
# --- | ||
# name: test_get_build_dirs[mock_repo1] | ||
list([ | ||
PosixPath('.'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a false positive. The problem is that get_build_dirs
function is not checking if the detected directory has a valid build. This PR should fix this issue: #135
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing that.
maven_deploy.dynamic_data["ci_services"] = [ci_info] | ||
assert check.run_check(maven_deploy, check_result) == CheckResultType.FAILED | ||
|
||
@pytest.fixture() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixture
is not needed. Why not to directly call BuildAsCodeCheck()
from the test function if nothing needs to be prepared before the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 29cdd3a.
tests/conftest.py
Outdated
CheckResult | ||
The CheckResult instance. | ||
""" | ||
return CheckResult(justification=[]) # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to instantiate CheckResult
directly in tests to set the expected output. This fixture is not doing much and I'm not sure if it's helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 29cdd3a.
root.add_callee(callee) | ||
github_actions_service.build_call_graph_from_node(callee) | ||
ci_info["callgraph"] = gh_cg | ||
assert build_as_code_check.run_check(gha_deploy, check_result) == CheckResultType.PASSED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a failed case too, for example change the workflow name to None and a wrong name and check that they fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 29cdd3a.
29cdd3a
to
52570e7
Compare
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
…as_code and build_service checks Signed-off-by: sophie-bates <sophie.bates@oracle.com>
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
Signed-off-by: sophie-bates <sophie.bates@oracle.com>
52570e7
to
8165079
Compare
Currently able to detect whether a Python project uses Pip or Poetry to manage its dependencies. This should be expanded in the future.
pyproject.toml
,setup.py
, orsetup.cfg
file.pyproject.toml
file, that we parse using tomllib to search for configuration settings for[tools.poetry]
.For the Build Service Check, the build commands that we currently support are:
pip install
poetry build
flit build
python setup.py
For the Build as Code Check, the deploy commands that we currently support are:
poetry publish
flit publish
twine upload
As well as the use of the pypa/gh-action-pypi-publish reusable workflow.