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

Fix load context bug #137

Merged
merged 10 commits into from
Jan 5, 2024
Merged

Fix load context bug #137

merged 10 commits into from
Jan 5, 2024

Conversation

mfleader
Copy link
Member

@mfleader mfleader commented Jan 3, 2024

Changes introduced with this PR

  • Fix bug
  • Add unit test

By contributing to this repository, I agree to the contribution guidelines.

@mfleader mfleader self-assigned this Jan 3, 2024
@mfleader mfleader marked this pull request as draft January 3, 2024 17:46
@mfleader mfleader marked this pull request as ready for review January 3, 2024 23:13
@mfleader mfleader linked an issue Jan 3, 2024 that may be closed by this pull request
loadfile/loadfile_test.go Outdated Show resolved Hide resolved
loadfile/loadfile.go Outdated Show resolved Hide resolved
cmd/arcaflow/main.go Outdated Show resolved Hide resolved
@mfleader mfleader marked this pull request as draft January 4, 2024 17:12
@mfleader mfleader marked this pull request as ready for review January 4, 2024 19:22
Copy link
Contributor

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something broke:

joconnel@joconnel-mac arcaflow-engine % export WFPATH=/Users/joconnel/Documents/projects/arcaflow/workflows/test-workflows/python_deployer_wait_simple
joconnel@joconnel-mac arcaflow-engine % go run cmd/arcaflow/main.go -config=$WFPATH/config.yaml -input=$WFPATH/input.yaml -context=$WFPATH
2024-01-04T15:45:44-05:00	error	source=main	Failed to load configuration file /Users/joconnel/Documents/projects/arcaflow/workflows/test-workflows/python_deployer_wait_simple/config.yaml (error loading context files, failed to read file /Users/joconnel/Documents/projects/arcaflow-clean/arcaflow-engine (read /Users/joconnel/Documents/projects/arcaflow-clean/arcaflow-engine: is a directory))

We've allowed specifying the entire directory, which, from that, it determined the workflow path without it being explicitly set. That appears to be broken now.

@mfleader
Copy link
Member Author

mfleader commented Jan 4, 2024

I initialized the slice to an incorrect length which added several empty strings that were converted to absolute file paths. It seems to be fixed now.

Copy link
Contributor

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and works.

Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a small question and a bunch of suggestions for polishing (but, I didn't see anything that I would block the merge over).

cmd/arcaflow/main.go Show resolved Hide resolved
cmd/arcaflow/main.go Outdated Show resolved Hide resolved
cmd/arcaflow/main.go Outdated Show resolved Hide resolved
loadfile/loadfile.go Outdated Show resolved Hide resolved
loadfile/loadfile.go Outdated Show resolved Hide resolved
loadfile/loadfile_test.go Outdated Show resolved Hide resolved
loadfile/loadfile_test.go Outdated Show resolved Hide resolved
loadfile/loadfile_test.go Outdated Show resolved Hide resolved
loadfile/loadfile_test.go Outdated Show resolved Hide resolved
loadfile/loadfile_test.go Outdated Show resolved Hide resolved
cmd/arcaflow/main.go Show resolved Hide resolved
cmd/arcaflow/main.go Outdated Show resolved Hide resolved
loadfile/loadfile.go Outdated Show resolved Hide resolved
loadfile/loadfile.go Outdated Show resolved Hide resolved
loadfile/loadfile.go Outdated Show resolved Hide resolved
@mfleader mfleader force-pushed the bug-load-context branch 2 times, most recently from 00da437 to c2acab8 Compare January 5, 2024 16:42
add readable test

refactor to load context test

changes

fix linting

add test explanation

refactor to limit context files read

change context establishment
rewrite test and add comment

fix init slice len

fix gosec warning maybe

report abs file path

config use abs filepath

use regular file

move code

clarify test

simplify error msg

func comment
Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, although I have a few comments and pointed questions for your consideration before you merge.

cmd/arcaflow/main.go Outdated Show resolved Hide resolved
loadfile/loadfile_test.go Outdated Show resolved Hide resolved
loadfile/loadfile_test.go Outdated Show resolved Hide resolved
loadfile/loadfile_test.go Outdated Show resolved Hide resolved
loadfile/loadfile_test.go Outdated Show resolved Hide resolved
cmd/arcaflow/main.go Outdated Show resolved Hide resolved
loadfile/loadfile.go Show resolved Hide resolved
cmd/arcaflow/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mfleader mfleader merged commit 8b971a1 into main Jan 5, 2024
5 checks passed
Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mfleader mfleader deleted the bug-load-context branch January 5, 2024 22:16
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.

Bug Loading Context Directory with Complex File Types
4 participants