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

Random Bug fixes #103

Merged
merged 6 commits into from
Dec 11, 2023
Merged

Random Bug fixes #103

merged 6 commits into from
Dec 11, 2023

Conversation

willtome
Copy link
Collaborator

@willtome willtome commented Oct 9, 2023

No description provided.

@youtous
Copy link
Contributor

youtous commented Dec 2, 2023

@willtome I have removed the Ansible-lint gh action and replaced it by pre-commit action.
The bug on the ansible-lint action that is preventing us to reference a specific version of the ansible-lint component is referenced by (ansible/ansible-lint#3830)

Using pre-commit as a single point of entry for the linting will ensure consistency for developments (locally and in the CI).

I have referenced the variable naming issue in #120, it will be treated as a separate PR.

Waiting for your review ;)

@jce-redhat
Copy link
Collaborator

Note: the YAML linter complains when comments aren't indented at the same level as the YAML item, which causes some of the lint issues. this should be configurable in the .yamllint config file, see about disabling this particular feature.

Copy link
Collaborator

@jce-redhat jce-redhat left a comment

Choose a reason for hiding this comment

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

LGTM with one overall comment. the roles that are under collections/ansible_collections/demo/compliance/ are copies of DISA-provided roles available from the DISA website. any time they get updated it may introduce more things that ansible-lint doesn't like, so we really shouldn't be linting them. that directory is supposed to be excluded in the ansible-lint config file, so i'm not sure why it would have complained.

since they're not content created for product-demos, we should probably move all of the roles out of collections/ and into roles/ as well, and update .ansible-lint appropriately.

@jce-redhat
Copy link
Collaborator

Note: the YAML linter complains when comments aren't indented at the same level as the YAML item, which causes some of the lint issues. this should be configurable in the .yamllint config file, see about disabling this particular feature.

that is, disable comments-indentation

@youtous
Copy link
Contributor

youtous commented Dec 9, 2023

TODO:

  • exclude content that is 3rd dep from lint
  • see if comment-indentation for yaml should be disabled: @jce-redhat, what is the rationale behind this?

thank you for the reviews and the comments

Copy link
Collaborator

@IPvSean IPvSean left a comment

Choose a reason for hiding this comment

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

I am good with this

@jce-redhat
Copy link
Collaborator

* [ ]  see if comment-indentation for yaml should be disabled: @jce-redhat, what is the rationale behind this?

the ansible-lint action was complaining about a couple of files where a block of YAML items was commented out, such as this:

this seems to be more of an issue with YAML embedded in python code rather than ansible tasks. by setting comments-indentation: disable set we can get past those action failures for now until the code gets cleaned up.

@jce-redhat jce-redhat closed this Dec 11, 2023
@jce-redhat
Copy link
Collaborator

@youtous for your TODOs, i opened #122 for the first one, and you could add the yamllint config change to #119 since it builds on this PR.

@jce-redhat jce-redhat reopened this Dec 11, 2023
@jce-redhat jce-redhat merged commit c0cd993 into main Dec 11, 2023
2 of 5 checks passed
@youtous youtous deleted the bug_fixes branch January 12, 2024 14:43
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.

4 participants