Skip to content

fix attempt to alter immutable DEFAULT_MASTER_OPTS - #58394

Closed
hwhesselink wants to merge 6 commits into
saltstack:masterfrom
hwhesselink:cloud_mopts_ro_fix
Closed

fix attempt to alter immutable DEFAULT_MASTER_OPTS#58394
hwhesselink wants to merge 6 commits into
saltstack:masterfrom
hwhesselink:cloud_mopts_ro_fix

Conversation

@hwhesselink

Copy link
Copy Markdown
Contributor

Dict salt.config.DEFAULT_MASTER_OPTS is a frozen immutable so the attempt to update it with "conf_path/master" causes an exception. This PR makes a copy and updates the copy.

What does this PR do?

What issues does this PR fix or reference?

Fixes:

Merge requirements satisfied?

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

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

Dict salt.config.DEFAULT_MASTER_OPTS is a frozen immutable so the attempt to update it with "conf_path/master" causes an exception. This PR makes a copy and updates the copy.
@hwhesselink
hwhesselink requested a review from a team as a code owner September 6, 2020 01:38
@ghost
ghost requested review from krionbsd and removed request for a team September 6, 2020 01:38
@krionbsd

krionbsd commented Sep 9, 2020

Copy link
Copy Markdown
Contributor

@hwhesselink thank you for contribution! Could you please run pre-commit -av and re-commit the changes? Could you please add a test case for this change?

@krionbsd krionbsd added the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Sep 9, 2020
@hwhesselink

Copy link
Copy Markdown
Contributor Author

Hi @krionbsd, a couple of questions: I'm not familiar with Jenkins but I installed pre-commit and ran "pre-commit run -av" in the cicd/jenkins directory and the 1-line diff is now +/- 200,000 lines so I suspect I'm not doing it right:-) How am I supposed to do this? Also, the error seems timing-related, I suspect the code in question only gets called when the new minion is slow, so I have no idea how to write a test for that.

@hwhesselink

Copy link
Copy Markdown
Contributor Author

Hi @krionbsd, any update on this?

s0undt3ch
s0undt3ch previously approved these changes Oct 17, 2020
@krionbsd krionbsd removed the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Oct 17, 2020
@ari

ari commented Aug 29, 2021

Copy link
Copy Markdown
Contributor

This problem appears to cause

[ERROR   ] There was a profile error: 'ImmutableDict' object has no attribute 'update'
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/salt/cloud/cli.py", line 290, in run
    ret = mapper.run_profile(self.options.profile, self.config.get("names"))
  File "/usr/local/lib/python3.7/site-packages/salt/cloud/__init__.py", line 1407, in run_profile
    ret[name] = self.create(vm_)
  File "/usr/local/lib/python3.7/site-packages/salt/cloud/__init__.py", line 1247, in create
    mopts_.update(
AttributeError: 'ImmutableDict' object has no attribute 'update'

and I cannot find a workaround to be able to use salt-cloud create

@ari

ari commented Aug 29, 2021

Copy link
Copy Markdown
Contributor

Sorry, I see. It is just sync_after_install which is not supported. If this is not going to be fixed, can we just remove sync_after_install from the documentation to save other people wasting their time?

@dwoz dwoz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs to be rebased.

@dwoz

dwoz commented Sep 8, 2021

Copy link
Copy Markdown
Contributor

@ari Are you willing to write a regression test for this? If so, we can get it into the next release.

@ari

ari commented Sep 8, 2021

Copy link
Copy Markdown
Contributor

I'm not the right person to work on this. My contribution here was nothing more than a suggestion to just remove the entire sync_after_install feature since its been broken for a couple of years, and no one seems to care too much. Just document the way to do this with the reactor.

@dwoz

dwoz commented Dec 10, 2023

Copy link
Copy Markdown
Contributor

Closing this due to inactivity. Anyone should feel free to re-open it if they want to see it through to the end in one release cycle.

@dwoz dwoz closed this Dec 10, 2023
@dwoz dwoz added help-wanted Community help is needed to resolve this Abandoned labels Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Abandoned help-wanted Community help is needed to resolve this

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants