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(gen-table): add script for inputs -> table #168

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zzehring
Copy link
Contributor

This adds a lightweight script for taking in a reusable workflow yaml file and outputting a markdown table of the inputs. It's a nice utility for quickly creating or updating docs for reusable workflows.

This adds a lightweight script for taking in a reusable workflow yaml
file and outputting a markdown table of the inputs. It's a nice utility
for quickly creating or updating docs for reusable workflows.
@zzehring zzehring requested a review from a team as a code owner June 27, 2024 20:32
Copy link
Member

@iainlane iainlane left a comment

Choose a reason for hiding this comment

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

noice

how about adding something into CI to lint/format this, too?

      - name: Lint Python
        uses: advanced-security/python-lint-code-scanning-action@e5446e09fc7d49dd4a1290193d45a41ee72a0a40 # v1.1.1
        with:
          linter: flake8

Now, about the language... Although I don't actually personally mind too much, after this is merged we'd have code written in

  • Go
  • Typescript
  • Python

in this repository. And I wonder if we should consolidate on two or if three is fine. I'll tag the recent contributors for their thoughts: @dsotirakis @zerok @ricky-undeadcoders

Comment on lines +47 to +51
if len(sys.argv) != 2:
print(__doc__)
sys.exit(1)
workflow_file = sys.argv[1]
main(Path(workflow_file))
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: might be a bit more robust to use argparse for this

Suggested change
if len(sys.argv) != 2:
print(__doc__)
sys.exit(1)
workflow_file = sys.argv[1]
main(Path(workflow_file))
parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter)
parser.add_argument("workflow_file", type=Path, help="Path to the workflow YAML file")
args = parser.parse_args()
main(args.workflow_file)

print("Could not find the inputs in the workflow file. Please ensure the workflow is a reusable one and has inputs.")
sys.exit(1)

# adding 2 for the backticks
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Maybe consider splitting the function here-ish and having a def generate_table(inputs: Dict[str, any]) -> None

Comment on lines +21 to +22
with open(workflow_file, "r") as f:
workflow = yaml.safe_load(f)
Copy link
Member

Choose a reason for hiding this comment

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

there are some errors to be caught here, e.g. FileNotFoundError, yaml.yamlError


## Prerequisites

- Python 3.x
Copy link
Member

Choose a reason for hiding this comment

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

pyyaml seems to be an external dependency. Somehow I don't have it installed 😮

print(f"| {'Name'.ljust(name_column_padding)} | {'Type'.ljust(type_column_padding)} | {'Description'.ljust(description_column_padding)} |")
print(f"| {'-' * name_column_padding} | {'-' * type_column_padding} | {'-' * description_column_padding} |")

for name, value in inputs.items():
Copy link
Member

Choose a reason for hiding this comment

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

question - should we do this

Suggested change
for name, value in inputs.items():
for name, value in sorted(inputs.items()):

or is it better to keep them in the same order as in the action?

workflow = yaml.safe_load(f)

try:
inputs = workflow[True]["workflow_call"]["inputs"]
Copy link
Member

Choose a reason for hiding this comment

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

main question: would it be hard to add support for our actions (not reusable workflows) in this tool too? it would be nice to have the same tables there - and we have more of those that we do reusable workflows!



def main(workflow_file_path: Path):
with open(workflow_file, "r") as f:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with open(workflow_file, "r") as f:
with open(workflow_file_path, "r") as f:

import sys
import yaml
from pathlib import Path
from pprint import pprint
Copy link
Member

Choose a reason for hiding this comment

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

debug leftover

Suggested change
from pprint import pprint

@dsotirakis
Copy link
Contributor

noice

how about adding something into CI to lint/format this, too?

      - name: Lint Python
        uses: advanced-security/python-lint-code-scanning-action@e5446e09fc7d49dd4a1290193d45a41ee72a0a40 # v1.1.1
        with:
          linter: flake8

Now, about the language... Although I don't actually personally mind too much, after this is merged we'd have code written in

  • Go
  • Typescript
  • Python

in this repository. And I wonder if we should consolidate on two or if three is fine. I'll tag the recent contributors for their thoughts: @dsotirakis @zerok @ricky-undeadcoders

I had the same thought about the language - I don't know if we want to add another language here, although looks pretty simple to do this in python. I wonder if a little bit more work (redoing this in Go) makes more sense too

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.

3 participants