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

Feature/generate defaults #81

Merged
merged 29 commits into from
Oct 10, 2024

Conversation

moritzrp
Copy link
Contributor

@moritzrp moritzrp commented Oct 6, 2024

This PR addresses #77 by adding a new command to generate defaults.

For writing defaults a new function will be created and to keep the
purpose clear "write" was renamed to "write_doc".
We need to check for None. A default value could for example be the
number 0. With just "if value" this would not pass and not write the
default.
Not sorting should be a saner default. That way the defaults will be in
the order as they are specified in the argument_specs.
This addresses yaml/pyyaml#234. PyYAML is
currently ignoring the indent for lists when dumping. Since ansible-lint
expects the indent, this is problematic.
@rndmh3ro
Copy link
Collaborator

rndmh3ro commented Oct 7, 2024

Thanks for your work! I took your comments from issue #77 and discuss them here:

Since --output-file is somewhat specific to docs, I added an option directly to the new command. If you want to write to a different file you can use aar-doc ROLE_PATH defaults --defaults-file /foo/bar/my_defaults.yml.

  • I'd like to have only one option, --output-file for this. I don't see a need to have output-file and defaults-file.
  • the options output_template and output_mode should then be moved to the markdown command, since they're specific to this command

If a later entry point configures an option with the same name it will be overwritten!

  • Fine for me I think, though this should be documented

Writes all collected defaults to ROLE_PATH/defaults/main.yml. If you specify a different file, it simply writes to said file.

  • should we use yaml or yml? Or detect the presence of either one and use it, if it exists? Seems best from a user perspective for me

The styling can be adjusted. For now the options are simply hard coded [...] All of these could also be controlled by the configuration or by an option passed along to the command.

  • let's do this if there's a need from someone. :)

When there are no options in the entrypoint, there's an error. We need a test for this and a fix. You can run the defaults command on the role tests/fixtures/roles/no_options/ to see the problem.


How about we add the description as comments atop the variables? ruamel.yaml can do this with yaml_set_comment_before_after_key, though I didn't try it yet.


There are some mypy and pylint-errors. If you use pre-commit locally, you'll get basic linting and tests.

> poetry run pylint aar_doc/
************* Module aar_doc.cli
aar_doc/cli.py:32:4: W1113: Keyword argument before variable positional arguments list in the definition of increase_indent function (keyword-arg-before-vararg)
aar_doc/cli.py:32:0: W0613: Unused argument 'args' (unused-argument)
aar_doc/cli.py:32:0: W0613: Unused argument 'kwargs' (unused-argument)
aar_doc/cli.py:83:4: W0621: Redefining name 'defaults' from outer scope (line 69) (redefined-outer-name)
aar_doc/cli.py:196:39: W0621: Redefining name 'defaults' from outer scope (line 69) (redefined-outer-name)
aar_doc/cli.py:470:4: W0621: Redefining name 'defaults' from outer scope (line 69) (redefined-outer-name)

> poetry run mypy aar_doc/
aar_doc/cli.py:196: error: Function "builtins.any" is not valid as a type  [valid-type]
aar_doc/cli.py:196: note: Perhaps you meant "typing.Any" instead of "any"?
aar_doc/cli.py:206: error: Name "output" already defined on line 203  [no-redef]
aar_doc/cli.py:464: error: Function "builtins.any" is not valid as a type  [valid-type]
aar_doc/cli.py:464: note: Perhaps you meant "typing.Any" instead of "any"?
Found 3 errors in 1 file (checked 2 source files)

@moritzrp
Copy link
Contributor Author

moritzrp commented Oct 7, 2024

There are some mypy and pylint-errors. If you use pre-commit locally, you'll get basic linting and tests.

Thanks for the hint! I completely missed the pre-commit config.

How about we add the description as comments atop the variables? ruamel.yaml can do this with yaml_set_comment_before_after_key, though I didn't try it yet.

I like the idea. It makes the defaults a lot clearer without having to reference the argument_specs. I will check out what needs to be done and how it plays with pyyaml being used.

should we use yaml or yml? Or detect the presence of either one and use it, if it exists? Seems best from a user perspective for me

I agree here as well. I think we could use what the user defined for theirmeta/main.yml and go with that extension. Otherwise, for the non-automated way, ansible-galaxy role init seems to be generating .yml by default. So personally, I would go with that.


As for the other topics, I will move the options and add the missing test case. I would also start adapting the README so it covers what is changed.

@rndmh3ro
Copy link
Collaborator

rndmh3ro commented Oct 7, 2024

I will check out what needs to be done and how it plays with pyyaml being used.

Pyyaml and ruamel.yaml are im my experience mostly compatible, so swapping out PyYAML for ruamel.yaml should be possible.

I agree here as well. I think we could use what the user defined for theirmeta/main.yml and go with that extension. Otherwise, for the non-automated way, ansible-galaxy role init seems to be generating .yml by default. So personally, I would go with that.

Agree. I just checked my locally existing Ansible-code and main.yml had 312 occurences, main.yaml only 7.

One last thing: if you want to (no need!), you could try moving the defaults and markdown code into separate files and subfolders - I think it makes sense now.

@moritzrp
Copy link
Contributor Author

moritzrp commented Oct 7, 2024

One last thing: if you want to (no need!), you could try moving the defaults and markdown code into separate files and subfolders - I think it makes sense now.

I'm glad you mention this actually. I was thinking about it since the cli.py is getting quite busy. But without the go ahead I didn't want to restructure parts that are not part of the feature 😄.

But I agree, it makes sense to have a defaults module and a markdown module.

@rndmh3ro
Copy link
Collaborator

rndmh3ro commented Oct 8, 2024

@moritzrp, if the PR is ready, you can set it to ready.

@moritzrp moritzrp marked this pull request as ready for review October 9, 2024 13:33
@moritzrp moritzrp requested a review from rndmh3ro as a code owner October 9, 2024 13:33
@moritzrp
Copy link
Contributor Author

moritzrp commented Oct 9, 2024

Hey @rndmh3ro , now the most most parts are done from my side.

the options output_template and output_mode should then be moved to the markdown command, since they're specific to this command

This is still open. Moving the options (and adjusting the test cases) still somehow breaks the test_output_template and the test_expand_home_path test case. At the moment I have no idea why that is.

The options have no negative effects on the defaults command. So from my side it would be fine to go without it for now. To change it I need to spend some time debugging what is going on there.

It's not specific to other changes I made. If you move the options on the main branch the same thing will happen.

@moritzrp
Copy link
Contributor Author

moritzrp commented Oct 9, 2024

Alright, now I am happy. I overlooked some changes from ruamel.yaml and finished up the structure.

@rndmh3ro
Copy link
Collaborator

Thanks for this PR! Awesome work.

I'll do a follow-up PR shortly to address some things that you can then review.

@rndmh3ro rndmh3ro added enhancement New feature or request minor labels Oct 10, 2024
@rndmh3ro rndmh3ro merged commit 1e3c181 into telekom-mms:main Oct 10, 2024
4 checks passed
@schurzi
Copy link
Contributor

schurzi commented Oct 10, 2024

Thank you, awesome work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants