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

Lint the results from role generation #1513

Merged
merged 2 commits into from
Feb 4, 2025
Merged

Lint the results from role generation #1513

merged 2 commits into from
Feb 4, 2025

Conversation

mabashian
Copy link
Member

@mabashian mabashian commented Jan 29, 2025

Jira Issue: https://issues.redhat.com/browse/AAP-37200

Description

This PR does two things:

  1. Introduces a linting post-processing step to the role gen file content.
  2. Accounts for warnings to be returned by wca api which was included in their demo last week. This should line up with warnings that come back from playbook generation

Testing

Unit test coverage added for new scenarios

Scenarios tested

None beyond the automated tests

Production deployment

  • This code change is ready for production on its own
  • This code change requires the following considerations before going to production:

@mabashian mabashian force-pushed the mabashian/AAP-37200 branch from 09cde98 to 0251a4c Compare February 3, 2025 15:29
@mabashian mabashian force-pushed the mabashian/AAP-37200 branch from 0251a4c to e3a0106 Compare February 3, 2025 18:51
@mabashian mabashian marked this pull request as ready for review February 3, 2025 19:00
@@ -507,7 +512,70 @@ def __init__(self, config: WCA_PIPELINE_CONFIGURATION):
super().__init__(config=config)

def invoke(self, params: RoleGenerationParameters) -> RoleGenerationResponse:
raise NotImplementedError
request = params.request
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should only be implemented in pipelines_saas.py since we don't want to provide this feature yet with the onPrem offert. @manstis WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@goneri @mabashian Yes, if this is only available for SaaS then that would be a better place for it. However please put a comment saying it should be moved back to the base WCA class when it becomes available for on-prem 😁 (otherwise we'll either have to remember or risk the duplication).

Copy link
Contributor

Choose a reason for hiding this comment

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

(I was going to review this PR tomorrow)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @manstis @goneri - I've moved this logic out to the saas pipeline

@mabashian mabashian force-pushed the mabashian/AAP-37200 branch from 3b9e74b to 14c441f Compare February 3, 2025 20:58
Copy link
Contributor

@manstis manstis left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mabashian mabashian merged commit 156aa6b into main Feb 4, 2025
11 checks passed
@mabashian mabashian deleted the mabashian/AAP-37200 branch February 4, 2025 14:35
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