Fixes include when using matrix and strategy build. #415
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SteffenSeckler
commented
Nov 11, 2020
| @@ -195,7 +192,7 @@ func (j *Job) GetMatrixes() []map[string]interface{} { | |||
|
|
|||
| func commonKeysMatch(a map[string]interface{}, b map[string]interface{}) bool { | |||
There was a problem hiding this comment.
Potentially, this entire function could be replaced with a reflect.DeepEqual(a, b).
What do you think?
I would replace the call to it in that case (l.175) and remove this function.
|
@SteffenSeckler - can you include some test cases for this? For examples of how to test sample workflow files, see: https://github.com/nektos/act/blob/master/pkg/runner/runner_test.go#L34 |
cplee
requested changes
Nov 30, 2020
There was a problem hiding this comment.
can you include some test cases for this? For examples of how to test sample workflow files, see: https://github.com/nektos/act/blob/master/pkg/runner/runner_test.go#L34
Prevent most "skipping unsupported platform messages".
# Conflicts: # pkg/runner/runner_test.go
cplee
approved these changes
Dec 8, 2020
edtan
added a commit
to edtan/act
that referenced
this pull request
Jan 23, 2021
This fixes nektos#499, where a matrix strategy with only include keys ends up causing multiple builds. This bugs appears to have been introduced in nektos#415, when extra include keys are added in the matrix strategy. The cause seems to be because the CartesianProduct function returns an item with empty keys, instead of return an empty set.
cplee
pushed a commit
that referenced
this pull request
Jan 23, 2021
This fixes #499, where a matrix strategy with only include keys ends up causing multiple builds. This bugs appears to have been introduced in #415, when extra include keys are added in the matrix strategy. The cause seems to be because the CartesianProduct function returns an item with empty keys, instead of return an empty set. Co-authored-by: Ed Tan <[email protected]>
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
includeof the matrix strategy was incorrectly handled.This simply appends the provided includes to the matrix product, s.t. this works (taken from the official doc)
Also replaces the comparison of the
commonKeysMatch()with areflect.DeepEqualto be able to compare maps.Potentially the entire loop could be replaced that way`
PS:
I do not know how testing works in this project, so I cannot easily add one. But I have seen, that neither
includenorexcludeappears to be tested.