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

Roll playbook explanation feedback choices back to previous values #1521

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

mabashian
Copy link
Member

This PR does not need a corresponding Jira item.

Description

There's a test downstream that's failing and I think it's due to this change that came in here: b2e04ee#diff-8b84a8269462c15e4258d8fc31d38ef8e231b71ddfd4e6155f3c705999e7d351L300-L302. I'm rolling that change back to see if we can get the tests happy.

Testing

Steps to test

  1. Pull down the PR
  2. ...
  3. ...

Scenarios tested

Production deployment

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

omaciel
omaciel previously approved these changes Feb 4, 2025
Copy link
Contributor

@omaciel omaciel left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

@omaciel omaciel left a comment

Choose a reason for hiding this comment

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

/lgtm

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.

I am hesitant that this is the correct fix.. and that using a 4 in the failing test would not be the more correct solution. VSCode sends possible values 0, 1, 2 or 3 for different user interactions.

In the meantime I'll submit a PR to ansible-wisdom-testing accordingly and we can choose which PR to merge later...

Looks like we'll need https://github.com/ansible/ansible-wisdom-testing/pull/855 merged/corrected first.

Unfortunately I think we'll need @goneri to confirm.

@manstis manstis requested a review from omaciel February 5, 2025 08:29
@manstis
Copy link
Contributor

manstis commented Feb 5, 2025

@goneri @mabashian I created a PR to fix the tests based on the regeneration of the client code used by the tests.

I was hoping to be able to push it into the existing PR.. but failed (I think there's a setting somewhere to allow others to contribute to existing PRs.. but my memory is a little hazzy). I've asked Goneri if he can cherry-pick it into his PR.

@manstis
Copy link
Contributor

manstis commented Feb 5, 2025

Closing as a duplication of #1523 😿

Sorry @mabashian your fix is the same; but I consider @goneri to be the "source of truth" for Playbook Explanation code.

Nothing personal.

@manstis manstis closed this Feb 5, 2025
@goneri goneri reopened this Feb 5, 2025
@goneri
Copy link
Contributor

goneri commented Feb 5, 2025

Closing as a duplication of #1523 😿

Sorry @mabashian your fix is the same; but I consider @goneri to be the "source of truth" for Playbook Explanation code.

Nothing personal.

Tssss, I closed mine. I was not paying attention and missed this PR. Thank you @mabashian for the fix!

Copy link
Contributor

@ldjebran ldjebran left a comment

Choose a reason for hiding this comment

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

/lgtm

@goneri goneri merged commit 66f92c3 into main Feb 5, 2025
14 of 16 checks passed
@goneri goneri deleted the mabashian/fix-playbook-explanation-choices branch February 5, 2025 17:22
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.

5 participants