-
Notifications
You must be signed in to change notification settings - Fork 623
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 static variable and resources support to ansible #4435
Conversation
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.
👍 Looks good to me! Reviewed everything up to f2aadd3 in 31 seconds
More details
- Looked at
464
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. backend/parsers/windmill-parser-yaml/src/lib.rs:97
- Draft comment:
Consider refactoringparse_ansible_arg_typ
andparse_ansible_typ
to reduce redundancy. Both functions have similar logic for handling YAML hash structures and extracting type information. A helper function could improve readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The code inparse_ansible_arg_typ
andparse_ansible_typ
functions is repetitive and can be refactored for better readability and maintainability. Theparse_ansible_arg_typ
function checks for specific types and returns anArgTyp
enum, whileparse_ansible_typ
returns aTyp
enum. Both functions have similar logic for handling YAML hash structures and extracting type information. This redundancy can be reduced by creating a helper function to handle the common logic.
2. backend/parsers/windmill-parser-yaml/src/lib.rs:75
- Draft comment:
Theget_default_for_typ
function assumes thedefault
field is always a string forwindmill_resource
. Consider handling different data types for robustness. - Reason this comment was not posted:
Confidence changes required:50%
Theget_default_for_typ
function has a potential issue where it assumes thedefault
field is always a string when thetype
iswindmill_resource
. This assumption might not hold if the YAML structure changes or if other types are introduced. It would be safer to handle different data types for thedefault
field.
3. backend/parsers/windmill-parser-yaml/src/lib.rs:362
- Draft comment:
Inparse_ansible_options
, consider storing the result ofcount_consecutive_vs
in a variable to avoid multiple calls for the same string. - Reason this comment was not posted:
Confidence changes required:33%
Theparse_ansible_options
function usescount_consecutive_vs
to determine verbosity, but this function is called multiple times for the same string. It would be more efficient to call it once and store the result in a variable.
4. backend/windmill-worker/src/ansible_executor.rs:432
- Draft comment:
The verbosity check inget_cmd_options
is redundant since it's already validated inparse_ansible_options
. Consider removing the check for efficiency. - Reason this comment was not posted:
Confidence changes required:33%
Theget_cmd_options
function inansible_executor.rs
is responsible for convertingAnsiblePlaybookOptions
into command-line arguments. The function currently checks verbosity by ensuring all characters are 'v' and the length is less than or equal to 6. This check is redundant because the verbosity is already validated inparse_ansible_options
.
Workflow ID: wflow_5Ym2wVNkcYc6s1Nb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Deploying windmill with
|
Latest commit: |
b41a5d0
|
Status: | ✅ Deploy successful! |
Preview URL: | https://689e9204.windmill.pages.dev |
Branch Preview URL: | https://win-399-ansible-variables-an.windmill.pages.dev |
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.
👍 Looks good to me! Incremental review on 127ec4c in 25 seconds
More details
- Looked at
125
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. cli/wasm/windmill_parser_wasm.generated.js:89
- Draft comment:
Ensure that the reordering of functions and imports does not affect the functionality of the wasm module. This might be due to a change in the build process or the underlying code that generates this file. - Reason this comment was not posted:
Confidence changes required:30%
The changes in the generated file seem to be related to the reordering of functions and imports. This might be due to a change in the build process or the underlying code that generates this file. It's important to ensure that these changes do not affect the functionality of the wasm module.
Workflow ID: wflow_OGmFq1kkHP1nu6dP
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on d51445d in 44 seconds
More details
- Looked at
84
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. cli/script_common.ts:48
- Draft comment:
Ensure file extensions are checked with a preceding dot for consistency and accuracy. This issue is also present on lines 51, 53, and 55. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_YGc3yuh3pTOJTBLf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on 56a9af7 in 39 seconds
More details
- Looked at
22
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_QqqoGHFbhFrCV06h
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on cad00e2 in 41 seconds
More details
- Looked at
20
lines of code in1
files - Skipped
1
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/package.json:461
- Draft comment:
Add a newline at the end of the file for consistency and to adhere to best practices. - Reason this comment was not posted:
Confidence changes required:50%
The newline at the end of the file is missing, which is a common best practice for text files.
Workflow ID: wflow_Lna2699j04iWV7rx
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on 83f30e5 in 32 seconds
More details
- Looked at
47
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. cli/metadata.ts:327
- Draft comment:
Ensure 'ansible' is supported in thefindClosestRawReqs
function to maintain consistency withupdateScriptLock
. - Reason this comment was not posted:
Comment did not seem useful.
2. cli/metadata.ts:495
- Draft comment:
The addition of ansible support ininferSchema
is consistent with the PR's intent. No issues found here. - Reason this comment was not posted:
Confidence changes required:0%
TheinferSchema
function has been updated to include ansible, which is consistent with the PR's intent to add ansible support.
Workflow ID: wflow_vN3yXjrm74FsTvXr
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on e76e590 in 1 minute and 14 seconds
More details
- Looked at
1153
lines of code in22
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. frontend/src/lib/components/ExecutionDuration.svelte:50
- Draft comment:
Ensure the interval is cleared when the component is destroyed to prevent potential memory leaks.
clearInterval(interval);
- Reason this comment was not posted:
Confidence changes required:50%
The code inExecutionDuration.svelte
has a potential issue with thesetInterval
function. If thejob
becomes aCompletedJob
, the interval is cleared, but if the job is not completed, the interval continues indefinitely. This could lead to memory leaks if the component is not destroyed properly. It's better to ensure that the interval is always cleared when the component is destroyed.
2. frontend/src/lib/components/FlowGraphViewerStep.svelte:106
- Draft comment:
Consider storing the conditionstepDetail.id !== 'preprocessor'
in a variable to avoid repetition.
const isNotPreprocessor = stepDetail.id !== 'preprocessor';
- Reason this comment was not posted:
Confidence changes required:30%
InFlowGraphViewerStep.svelte
, the code checks forstepDetail.id !== 'preprocessor'
multiple times. It would be more efficient to store this condition in a variable and reuse it.
3. frontend/src/lib/components/FlowJobResult.svelte:41
- Draft comment:
Remove the console log statement from thegetLogs
function for cleaner production code. - Reason this comment was not posted:
Confidence changes required:20%
InFlowJobResult.svelte
, thegetLogs
function has a console log statement that seems to be left for debugging purposes. It should be removed for cleaner production code.
4. frontend/src/lib/components/TestJobLoader.svelte:106
- Draft comment:
Remove the console log statement from thegetLogs
function for cleaner production code. - Reason this comment was not posted:
Confidence changes required:20%
InTestJobLoader.svelte
, thegetLogs
function has a console log statement that seems to be left for debugging purposes. It should be removed for cleaner production code.
5. frontend/src/lib/components/flows/FlowProgressBar.svelte:68
- Draft comment:
Simplify the use ofMath.max
with a single argument inupdateJobProgress
function.
subLength = subStepLength || 1;
- Reason this comment was not posted:
Confidence changes required:30%
InFlowProgressBar.svelte
, theupdateJobProgress
function usesMath.max
with a single argument. This is unnecessary as it will always return the argument itself. It should be simplified.
Workflow ID: wflow_N2y9nZIiKfsPQkkj
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on 5f10465 in 27 seconds
More details
- Looked at
45
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. cli/build.sh:3
- Draft comment:
Add a newline at the end of the file to follow best practices for POSIX compliance. - Reason this comment was not posted:
Confidence changes required:50%
The shebang line is a good practice for scripts to specify the interpreter, but the newline at the end of the file is missing, which is a common best practice.
2. cli/gen_wm_client.sh:23
- Draft comment:
Add a newline at the end of the file to follow best practices for POSIX compliance. - Reason this comment was not posted:
Confidence changes required:50%
The shebang line is a good practice for scripts to specify the interpreter, but the newline at the end of the file is missing, which is a common best practice.
3. cli/metadata.ts:315
- Draft comment:
Consider using optional chaining and nullish coalescing for cleaner code:
metadataContent.no_main_func = result.no_main_func ?? metadataContent.no_main_func;
- Reason this comment was not posted:
Confidence changes required:30%
The updateScriptSchema function has been modified to check for null values before assigning them to metadataContent. This is a good practice to avoid overwriting existing values with null. However, the code could be slightly optimized for readability.
Workflow ID: wflow_jVyDNptmRYhaUkIH
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Closes #4370 |
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.
👍 Looks good to me! Incremental review on a365b26 in 57 seconds
More details
- Looked at
686
lines of code in10
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. cli/gen/core/CancelablePromise.ts:84
- Draft comment:
Typo in theSymbol.toStringTag
getter. It should be 'Cancelable Promise' instead of 'Cancellable Promise'.
return "Cancelable Promise";
- Reason this comment was not posted:
Confidence changes required:10%
The code incli/gen/core/CancelablePromise.ts
has a typo in theSymbol.toStringTag
getter. The correct spelling should be 'Cancelable Promise' instead of 'Cancellable Promise'. This typo is minor but should be corrected for consistency and clarity.
2. cli/gen/core/request.ts:257
- Draft comment:
Typo in the error message for HTTP status code 418. It should be 'I'm a teapot' instead of 'Im a teapot'.
417: 'Expectation Failed',
418: 'I\'m a teapot',
- Reason this comment was not posted:
Confidence changes required:10%
ThecatchErrorCodes
function incli/gen/core/request.ts
has a typo in the error message for HTTP status code 418. It should be 'I'm a teapot' instead of 'Im a teapot'. This is a minor issue but should be corrected for accuracy.
Workflow ID: wflow_IFwJ69CvKK5FHaU6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
Skipped PR review on f72fd70 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
Generated with ❤️ by ellipsis.dev
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.
👍 Looks good to me! Incremental review on b41a5d0 in 28 seconds
More details
- Looked at
532
lines of code in15
files - Skipped
1
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_Vh3yxbQISJfsGN9q
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
You can specify static variables and resources like this:
They will not appear on the frontend UI but still be passed as extra-vars to the ansible playbook
Also you can specify some options:
Various CLI fixes
Important
Add support for static variables, resources, and execution options in Ansible playbooks via YAML configuration.
parse_ansible_sig()
andparse_ansible_reqs()
.parse_ansible_options()
.parse_ansible_arg_typ()
,get_default_for_typ()
, andyaml_to_json()
added to handle new YAML structures inlib.rs
.get_cmd_options()
inansible_executor.rs
to convert options to command-line arguments.AnsiblePlaybookOptions
andAnsibleRequirements
updated to include new fields for options, vars, and resources.inferContentTypeFromFilePath()
inscript_common.ts
.ZipFSElement()
insync.ts
to handle Ansible playbooks.pkg-yaml
inpublish-pkgs.sh
.This description was created by
for b41a5d0. It will automatically update as commits are pushed.