Skip to content
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

add depends_on; fix execution_order_group 0. #320

Merged
merged 4 commits into from
Apr 25, 2024

Conversation

pseudomorph
Copy link
Collaborator

Pull Request

Related Github Issues

Description

Adds functionality for depends_on
Fixes bug where the base execution_group (0) was not explicitly given.

Security Implications

  • [none]

System Availability

  • [none]

@pseudomorph pseudomorph force-pushed the add_depends_on branch 2 times, most recently from 94fceef to 504e932 Compare March 5, 2024 05:11
@pseudomorph
Copy link
Collaborator Author

Blocked by: #311

@Almenon
Copy link
Collaborator

Almenon commented Apr 13, 2024

I just merged in #311, can you rebase please? That might fix the failing windows test.

@Almenon
Copy link
Collaborator

Almenon commented Apr 13, 2024

I reviewed the code, it looks good but I'll hold off on approving it until the tests pass. Also if you could update the readme to add the new flag that would be great.

@pseudomorph
Copy link
Collaborator Author

@Almenon - Any idea why tests would be failing for Windows?

@Almenon
Copy link
Collaborator

Almenon commented Apr 21, 2024

Not sure. Based on the diff it looks like DependsOn should be a list with three elements, but for some reason it's missing one of the expected elements "depender_on_depender_nested". The test fails on my machine too, so it's not just a CI-specific error.

@Almenon
Copy link
Collaborator

Almenon commented Apr 21, 2024

It's also weird because in your code I don't see anything specific to windows / linux 🤔

@jon-walton
Copy link

Here's a patch to get you started.

diff --git a/cmd/generate.go b/cmd/generate.go
index f274f83..70b7532 100644
--- a/cmd/generate.go
+++ b/cmd/generate.go
@@ -828,7 +828,7 @@ func main(cmd *cobra.Command, args []string) error {
                                dependsOnList := []string{}
                                // choose order group based on dependencies
                                for _, dep := range project.Autoplan.WhenModified {
-                                       depPath := filepath.Dir(filepath.Join(project.Dir, dep))
+                                       depPath := filepath.ToSlash(filepath.Dir(filepath.Join(project.Dir, dep)))
                                        if depPath == project.Dir {
                                                // skip dependency on oneself
                                                continue

A project's WhenModified is forced with unix path separators via relativeDependencies = append(relativeDependencies, filepath.ToSlash(relativePath)) yet, we're comparing it against depPath which was made with OS specific path separators, quite subtle

The tests TestWithExecutionOrderGroupsAndDependsOn and TestWithDependsOn now pass for me

@pseudomorph
Copy link
Collaborator Author

Thanks @jon-walton !

@Almenon Almenon merged commit bec44ce into transcend-io:master Apr 25, 2024
3 checks passed
@Almenon
Copy link
Collaborator

Almenon commented Apr 25, 2024

Merged, but the release CI process for releasing a new build with this change is broken. Can you review #332 please? Hopefully that will fix it.

@Almenon
Copy link
Collaborator

Almenon commented Apr 28, 2024

v1.18.0 is out with this change now :)

@Almenon Almenon added this to the v1.18.0 milestone Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants