Fix for the workflow cli create cmd#1294
Conversation
davehadley
left a comment
There was a problem hiding this comment.
Thanks @hazdl for implementing this fix.
There seems to be refactoring in the PR as well a bug fix. Where possible, it's easier to review if you do the minimal changes required to fix the bug, and the refactor as a separate PR.
In the workflows repo we follow conventional commits. See: https://www.conventionalcommits.org/en/v1.0.0/. And we normally merge PRs without squashing (i.e. the PR commit history will get linearly merged into main).
I suggest squashing this PRs commits into 1 or 2 commits: eg
"fix(workflows-cli): summary of bug fixed"
"refactor(workflows-cli): summary of refactoring done"
Thanks again for fixing this! 👍
| # auth-daemon | ||
|
|
||
| Sidecar HTTP proxy that injects valid OIDC access tokens into GraphQL requests made from within Argo workflows. Workflows call the daemon on `localhost` instead of the GraphQL API directly; the daemon handles token refresh and auth header injection transparently. | ||
| Sidecar HTTP proxy that injects valid OIDC access tokens into GraphQL requests made from within Argo workflows. Workflows call the daemon on `localhost` instead of the GraphQL API directly; the daemon handles token refresh and auth header injection transparently. |
There was a problem hiding this comment.
this edit seems un-related to this pull request?
| let target = fs::read_link(&src_path) | ||
| .map_err(|e| format!("Failed to read symlink {}: {}", src_path.display(), e))?; | ||
|
|
||
| fs_sym::symlink(&target, &dest_path) |
There was a problem hiding this comment.
you seem to have lost #[cfg(unix)]?
| @@ -19,38 +19,66 @@ pub fn create(args: CreateArgs) { | |||
| } | |||
|
|
|||
| fn generate_template_repo(args: &CreateArgs, prompt_fn: fn(&str) -> bool) -> Result<(), String> { | |||
There was a problem hiding this comment.
There's a lot of change here, some of it seems to be refactoring.
What actually was the cause of the bug? And what was the fix?
Is it possible to create a unit test for this bug to prevent future regressions?
| @@ -0,0 +1,26 @@ | |||
|
|
|||
There was a problem hiding this comment.
Is this necessary for this PR? I don't think that it shouldn't be included in the examples/conventional-templates directory.
- This is not a
ClusterWorkflowTemplate, it is aWorkflow. These are distinct concepts: - Templates in the
examplesdirectory are user facing. That will get ingested by ArgoCD and appear at https://workflows.diamond.ac.uk where users and see it and run it. - It would be better to have an automated rust unit test to test CLI functionality. Not a Workflow that you have to manually submit to the cluster,
Fixed the issues in the cli create cmd. Under the issue :AP:1099.