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

[3007.x] Fix minion config option startup_states #66605

Open
wants to merge 4 commits into
base: 3007.x
Choose a base branch
from

Conversation

transistortim
Copy link

@transistortim transistortim commented May 30, 2024

What does this PR do?

The changes in this PR fix #66592 by making additional functions use async/await.

What issues does this PR fix or reference?

Fixes #66592

Previous Behavior

11a06ce made some internal functions async. This broke functions calling these internal functions without await. This leads to the minion configuration option startup_states not working any more.

New Behavior

Starting the minion with startup_states now works as expected.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

No. Is this required?

Note

Making more functions async might have introduced some more bugs I didn't catch yet. Specifically I've never used ProxyMinions and Syndic, maybe somebody with more experience could look into that.

@transistortim transistortim requested a review from a team as a code owner May 30, 2024 20:56
Copy link

welcome bot commented May 30, 2024

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Fix minion config option startup_states [master] Fix minion config option startup_states May 30, 2024
@dmurphy18 dmurphy18 requested a review from dwoz June 10, 2024 15:51
@twangboy twangboy added the test:full Run the full test suite label Jun 12, 2024
Copy link
Contributor

@twangboy twangboy left a comment

Choose a reason for hiding this comment

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

This should probably be made against 3007.x branch. Would you mind rebasing this PR?

@transistortim
Copy link
Author

Thanks for the review! I'm currently on vacation, I'll take care of it when I get back (around June 24).

@73VW
Copy link

73VW commented Aug 8, 2024

Hey @transistortim any update on this?

Thanks

@transistortim transistortim changed the base branch from master to 3007.x August 11, 2024 19:20
@transistortim transistortim requested a review from a team as a code owner August 11, 2024 19:20
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title [master] Fix minion config option startup_states [3007.x] Fix minion config option startup_states Aug 11, 2024
@transistortim
Copy link
Author

Sorry for the delay, I was busy with some other things. Finally, here's the update :)

@dwoz dwoz requested a review from twangboy October 21, 2024 21:26
@amalaguti
Copy link

Will this fix be made available for 3007.2 ?

@twangboy
Copy link
Contributor

twangboy commented Oct 30, 2024

Looks like there is a lint failure. Would you mind taking a look?
Also, there are some test failures.

@amalaguti
Copy link

Looks like there is a lint failure. Would you mind taking a look? Also, there are some test failures.

Shane,
found this in the CI pipeline

************* Module salt.minion
salt/minion.py:3879: [W0236(invalid-overridden-method), ProxyMinion.tune_in] Method 'tune_in' was expected to be 'async', found it instead as 'non-async'

Today I worked on another two issues related to async/await and also the tune_in() (the main tune_in in minion class).
Looks like it's all related.
#66932
#67016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test:full Run the full test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] [3007.1] startup_states: highstate stop working
4 participants