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

feat: add static variable and resources support to ansible #4435

Merged
merged 11 commits into from
Sep 25, 2024

Conversation

wendrul
Copy link
Contributor

@wendrul wendrul commented Sep 24, 2024

You can specify static variables and resources like this:

extra_vars:
  inventroy_resource:
    type: windmill_resource
    resource: u/admin/other_inventory
  my_var:
    type: windmill_variable
    variable: u/admin/beneficent_variable

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:

options:
  - vvv #verbosity
  - forks: 5
  - flush_cache

Various CLI fixes


Important

Add support for static variables, resources, and execution options in Ansible playbooks via YAML configuration.

  • Behavior:
    • Support for static variables and resources in Ansible playbooks via YAML in parse_ansible_sig() and parse_ansible_reqs().
    • Options like verbosity and forks can be specified in YAML and parsed in parse_ansible_options().
  • Functions:
    • parse_ansible_arg_typ(), get_default_for_typ(), and yaml_to_json() added to handle new YAML structures in lib.rs.
    • get_cmd_options() in ansible_executor.rs to convert options to command-line arguments.
  • Structures:
    • AnsiblePlaybookOptions and AnsibleRequirements updated to include new fields for options, vars, and resources.
  • Misc:
    • Added Ansible support to inferContentTypeFromFilePath() in script_common.ts.
    • Updated ZipFSElement() in sync.ts to handle Ansible playbooks.
    • Published pkg-yaml in publish-pkgs.sh.

This description was created by Ellipsis for b41a5d0. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Reviewed everything up to f2aadd3 in 31 seconds

More details
  • Looked at 464 lines of code in 2 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 refactoring parse_ansible_arg_typ and parse_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 in parse_ansible_arg_typ and parse_ansible_typ functions is repetitive and can be refactored for better readability and maintainability. The parse_ansible_arg_typ function checks for specific types and returns an ArgTyp enum, while parse_ansible_typ returns a Typ 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:
    The get_default_for_typ function assumes the default field is always a string for windmill_resource. Consider handling different data types for robustness.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The get_default_for_typ function has a potential issue where it assumes the default field is always a string when the type is windmill_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 the default field.
3. backend/parsers/windmill-parser-yaml/src/lib.rs:362
  • Draft comment:
    In parse_ansible_options, consider storing the result of count_consecutive_vs in a variable to avoid multiple calls for the same string.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The parse_ansible_options function uses count_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 in get_cmd_options is redundant since it's already validated in parse_ansible_options. Consider removing the check for efficiency.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The get_cmd_options function in ansible_executor.rs is responsible for converting AnsiblePlaybookOptions 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 in parse_ansible_options.

Workflow ID: wflow_5Ym2wVNkcYc6s1Nb


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

cloudflare-workers-and-pages bot commented Sep 24, 2024

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

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

View logs

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on 127ec4c in 25 seconds

More details
  • Looked at 125 lines of code in 1 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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on d51445d in 44 seconds

More details
  • Looked at 84 lines of code in 4 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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on 56a9af7 in 39 seconds

More details
  • Looked at 22 lines of code in 2 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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on cad00e2 in 41 seconds

More details
  • Looked at 20 lines of code in 1 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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on 83f30e5 in 32 seconds

More details
  • Looked at 47 lines of code in 1 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 the findClosestRawReqs function to maintain consistency with updateScriptLock.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. cli/metadata.ts:495
  • Draft comment:
    The addition of ansible support in inferSchema is consistent with the PR's intent. No issues found here.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The inferSchema 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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on e76e590 in 1 minute and 14 seconds

More details
  • Looked at 1153 lines of code in 22 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 in ExecutionDuration.svelte has a potential issue with the setInterval function. If the job becomes a CompletedJob, 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 condition stepDetail.id !== 'preprocessor' in a variable to avoid repetition.
const isNotPreprocessor = stepDetail.id !== 'preprocessor';
  • Reason this comment was not posted:
    Confidence changes required: 30%
    In FlowGraphViewerStep.svelte, the code checks for stepDetail.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 the getLogs function for cleaner production code.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    In FlowJobResult.svelte, the getLogs 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 the getLogs function for cleaner production code.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    In TestJobLoader.svelte, the getLogs 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 of Math.max with a single argument in updateJobProgress function.
subLength = subStepLength || 1;
  • Reason this comment was not posted:
    Confidence changes required: 30%
    In FlowProgressBar.svelte, the updateJobProgress function uses Math.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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on 5f10465 in 27 seconds

More details
  • Looked at 45 lines of code in 3 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.

@wendrul
Copy link
Contributor Author

wendrul commented Sep 25, 2024

Closes #4370

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on a365b26 in 57 seconds

More details
  • Looked at 686 lines of code in 10 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 the Symbol.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 in cli/gen/core/CancelablePromise.ts has a typo in the Symbol.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%
    The catchErrorCodes function in cli/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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on b41a5d0 in 28 seconds

More details
  • Looked at 532 lines of code in 15 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.

@rubenfiszel rubenfiszel merged commit 398a09b into main Sep 25, 2024
4 checks passed
@rubenfiszel rubenfiszel deleted the win-399-ansible-variables-and-secrets branch September 25, 2024 13:05
@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants