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.

Sign up for GitHub

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

Jump to bottom

Schedule rruleset fix related #13446 #13611

Merged
merged 5 commits into from
Aug 16, 2023
Merged

Schedule rruleset fix related #13446 #13611

merged 5 commits into from
Aug 16, 2023

Conversation

KaraokeKev
Copy link
Contributor

@KaraokeKev KaraokeKev commented Feb 21, 2023 •
edited
Loading

SUMMARY

Fix to process_list function. isinstance checks for list, but list is desired end state. Changed to string. Related #13446

In addition, added .lower() to value definition to allow for case-insensitivity from user input.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • Collection
AWX VERSION
awx: 0.1.dev32853+g37c7a0c
ADDITIONAL INFORMATION

Case 1

rule_parameters:
        - frequency: 'day'
          interval: 1
        - frequency: 'day'
          interval: 1
          byweekday: sunday
          include: False

Prior to change:

fatal: [localhost]: FAILED! => {"msg": "An unhandled exception occurred while running the lookup plugin 'awx.awx.schedule_rruleset'. Error was a <class 'ansible.errors.AnsibleError'>, original message: In rule 2 byweekday must only contain values in monday, tuesday, wednesday, thursday, friday, saturday, sunday. In rule 2 byweekday must only contain values in monday, tuesday, wednesday, thursday, friday, saturday, sunday"}

Following change:

ok: [localhost] => {
    "msg": "DTSTART;TZID=America/New_York:20220919T042000 RRULE:FREQ=DAILY;INTERVAL=1 EXRULE:FREQ=DAILY;BYDAY=SU;INTERVAL=1"
}

Case 2

rule_parameters:
        - frequency: 'day'
          interval: 1
        - frequency: 'day'
          interval: 1
          byweekday: sunday,tuesday
          include: False

Prior to change:

fatal: [localhost]: FAILED! => {"msg": "An unhandled exception occurred while running the lookup plugin 'awx.awx.schedule_rruleset'. Error was a <class 'ansible.errors.AnsibleError'>, original message: In rule 2 byweekday must only contain values in monday, tuesday, wednesday, thursday, friday, saturday, sunday. In rule 2 byweekday must only contain values in monday, tuesday, wednesday, thursday, friday, saturday, sunday"}

Following change:

ok: [localhost] => {
    "msg": "DTSTART;TZID=America/New_York:20220919T042000 RRULE:FREQ=DAILY;INTERVAL=1 EXRULE:FREQ=DAILY;BYDAY=TU,SU;INTERVAL=1"
}

Case 3

rule_parameters:
        - frequency: 'day'
          interval: 1
        - frequency: 'day'
          interval: 1
          byweekday: ['sunday', 'tuesday']
          include: False

Prior to change:

fatal: [localhost]: FAILED! => {"msg": "An unhandled exception occurred while running the lookup plugin 'awx.awx.schedule_rruleset'. Error was a <class 'AttributeError'>, original message: 'list' object has no attribute 'split'. 'list' object has no attribute 'split'"}

Following change:

ok: [localhost] => {
    "msg": "DTSTART;TZID=America/New_York:20220919T042000 RRULE:FREQ=DAILY;INTERVAL=1 EXRULE:FREQ=DAILY;BYDAY=TU,SU;INTERVAL=1"
}

Case 4

rule_parameters:
        - frequency: 'day'
          interval: 1
        - frequency: 'day'
          interval: 1
          byweekday: sunday, monday, Tuesday
          include: False

Prior to change:

fatal: [localhost]: FAILED! => {"msg": "An unhandled exception occurred while running the lookup plugin 'awx.awx.schedule_rruleset'. Error was a <class 'ansible.errors.AnsibleError'>, original message: In rule 2 byweekday must only contain values in monday, tuesday, wednesday, thursday, friday, saturday, sunday. In rule 2 byweekday must only contain values in monday, tuesday, wednesday, thursday, friday, saturday, sunday"}

Following change:

ok: [localhost] => {
    "msg": "DTSTART;TZID=America/New_York:20220919T042000 RRULE:FREQ=DAILY;INTERVAL=1 EXRULE:FREQ=DAILY;BYDAY=MO,TU,SU;INTERVAL=1"
}

@github-actions github-actions bot added component:awx_collection issues related to the collection for controlling AWX community labels Feb 21, 2023
@KaraokeKev KaraokeKev force-pushed the schedule_rruleset-fix branch 2 times, most recently from 038bb73 to 8c79900 Compare February 21, 2023 23:02
@fosterseth
Copy link
Member

@KaraokeKev thanks for this PR, do you mind adding a couple of test cases to this file (to assert that the lookup is successful)

https://github.com/ansible/awx/blob/devel/awx_collection/tests/integration/targets/lookup_rruleset/tasks/main.yml

@fosterseth
Copy link
Member

duplicate PR (that aims to fix the same issue) is here #13541

We plan to merge that in, so this PR may no longer be needed.

@KaraokeKev
Copy link
Contributor Author

I would note that while #13541 does solve the base bug, it does not allow for the passing of a list or case insensitivity.

@AlanCoding
Copy link
Member

If we could get a test case added to awx_collection/tests/integration/targets/lookup_rruleset/tasks/main.yml which demonstrates the case insensitivity bug then that should allow us to tease apart the unique fixes here eventually.

@KaraokeKev
Copy link
Contributor Author

If we could get a test case added to awx_collection/tests/integration/targets/lookup_rruleset/tasks/main.yml which demonstrates the case insensitivity bug then that should allow us to tease apart the unique fixes here eventually.

Added.

AlanCoding
AlanCoding reviewed Feb 23, 2023
View reviewed changes
awx_collection/plugins/lookup/schedule_rruleset.py Outdated
@@ -210,10 +210,10 @@ def process_integer(self, field_name, rule, min_value, max_value, rule_number):

def process_list(self, field_name, rule, valid_list, rule_number):
return_values = []
if isinstance(rule[field_name], list):
if isinstance(rule[field_name], str):
Copy link
Member

Choose a reason for hiding this comment

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

I don't agree with this. If you look elsewhere in this file you find that this line is duplicated, and the other case has an associated comment, see #13541 (comment) for my approach to address the original intent. Best to abandon this here.

@AlanCoding
Copy link
Member

This has conflicts due to #13541, but I hope the issues are easy to follow from my review comments here. I would love to get this rebased and then merged.

@rebeccahhh
Copy link
Member

@KaraokeKev would you like to do the rebase? If not a member of our team can try to get around to this at some point in time (it will probably be merged faster if you do it though).

@gamuniz gamuniz assigned gamuniz and djyasin and unassigned gamuniz and djyasin Jul 12, 2023
@KaraokeKev
Copy link
Contributor Author

@KaraokeKev would you like to do the rebase? If not a member of our team can try to get around to this at some point in time (it will probably be merged faster if you do it though).

Rebase completed

@jay-steurer jay-steurer self-assigned this Aug 9, 2023
@jay-steurer jay-steurer merged commit b8ec7c4 into ansible:devel Aug 16, 2023
14 checks passed
djyasin pushed a commit to djyasin/awx that referenced this pull request Sep 16, 2024
@KaraokeKev @jay-steurer @djyasin
Schedule rruleset fix related ansible#13446 (ansible#13611)
5c3c244 
Signed-off-by: Kevin Pavon <[email protected]>
Co-authored-by: Jessica Steurer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlanCoding AlanCoding AlanCoding left review comments

@jay-steurer jay-steurer jay-steurer approved these changes

@djyasin djyasin djyasin approved these changes

Assignees

@jay-steurer jay-steurer

@djyasin djyasin

Labels
community component:awx_collection issues related to the collection for controlling AWX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@KaraokeKev @fosterseth @AlanCoding @rebeccahhh @jay-steurer @djyasin @gamuniz