-
-
Notifications
You must be signed in to change notification settings - Fork 10
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 matrix strategy #118
Conversation
While the combination logic successfully creates all 6 combinations, the output seems to be referencing the matrix variables without properly substituting their values. The output looks like this for each combination:
Tried using Additionally, commands like |
hi @abhijeetSaroha, thanks for working on that! it seems it is almost ready to go :) the jinja template is defined as it fails because it is trying to process, but there are some errors inside the code. check all the usage of as you can see there, it sets the variables for let me know if you need any more details please |
It works fine now as it should be. I also break the function https://github.com/abhijeetSaroha/makim/blob/matrix-strategy/src/makim/core.py#L455C2-L480C42 |
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.
@abhijeetSaroha , thanks for working on that.
in general it looks good to me .. just a few changes, I will also paste here the complete diff:
diff --git a/src/makim/core.py b/src/makim/core.py
index 06e1f7a..cd6fb11 100644
--- a/src/makim/core.py
+++ b/src/makim/core.py
@@ -33,6 +33,7 @@ from makim.logs import MakimError, MakimLogs
AppConfigType: TypeAlias = Dict[str, Union[str, List[str]]]
+
SCOPE_GLOBAL = 0
SCOPE_GROUP = 1
SCOPE_TARGET = 2
@@ -495,7 +496,7 @@ class Makim:
List of dictionaries, each containing one possible combination
"""
if not matrix_config:
- return [{}]
+ return []
# Convert matrix config into format suitable for product
keys = list(matrix_config.keys())
@@ -588,7 +589,7 @@ class Makim:
)
# Get matrix configuration if it exists
- matrix_combinations: list[dict[str, Any]] = [{}]
+ matrix_combinations: list[dict[str, Any]] = []
if matrix_config := self.task_data.get('matrix', {}):
matrix_combinations = self._generate_matrix_combinations(
matrix_config
@@ -631,7 +632,7 @@ class Makim:
width, _ = get_terminal_size()
# Run command for each matrix combination
- for matrix_vars in matrix_combinations:
+ for matrix_vars in matrix_combinations or [{}]:
# Update environment variables
for k, v in env.items():
os.environ[k] = v
also please rebase your branch so you can get the changes that fixes the linter on ci.
src/makim/core.py
Outdated
for k, v in env.items(): | ||
os.environ[k] = v | ||
# Get matrix configuration if it exists | ||
matrix_combinations: list[dict[str, Any]] = [{}] |
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.
matrix_combinations: list[dict[str, Any]] = [{}] | |
matrix_combinations: list[dict[str, Any]] = [] |
src/makim/core.py
Outdated
List of dictionaries, each containing one possible combination | ||
""" | ||
if not matrix_config: | ||
return [{}] |
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.
return [{}] | |
return [] |
src/makim/core.py
Outdated
MakimLogs.print_info( | ||
'TARGET: ' + f'{self.group_name}.{self.task_name}' | ||
# Run command for each matrix combination | ||
for matrix_vars in matrix_combinations: |
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 matrix_vars in matrix_combinations: | |
for matrix_vars in matrix_combinations or [{}]: |
@xmnlab, I have made the changes and rebase the branch too. |
thank you @abhijeetSaroha ! great job! |
🎉 This PR is included in version 1.18.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Add support of matrix.
Solves #81