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

fix file.managed creating temporary files in cwd #63892

Closed
wants to merge 2 commits into from

Conversation

Obbi89
Copy link

@Obbi89 Obbi89 commented Mar 16, 2023

tempfile.mkstemp internally calls os.path.abspath on its dir argument, causing "" to be converted to the working directory of the process.

What does this PR do?

Change default for mkstemp call in state file.managed to prevent creation of temporary files in cwd.

What issues does this PR fix or reference?

Fixes: #63877

Previous Behavior

A file.managed state with check_cmd enabled will create temporary files in the processes working directory

New Behavior

A file.managed state with check_cmd enabled will create temporary files in the typical temporary folders - following the algorithm specified in pythons tempfile documentation.

Merge requirements satisfied?

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

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

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

@Obbi89 Obbi89 requested a review from a team as a code owner March 16, 2023 13:47
@Obbi89 Obbi89 requested review from dwoz and removed request for a team March 16, 2023 13:47
@welcome
Copy link

welcome bot commented Mar 16, 2023

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 [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@Obbi89
Copy link
Author

Obbi89 commented Mar 16, 2023

do we need a Testcase here? (the famous single line change, what bad could that ever do 🙃 )
The change shouldn't need any changelog mention either 🤷 as it doesn't change anything noticeable anyways.

@dwoz
Copy link
Contributor

dwoz commented Dec 12, 2023

@Obbi89 are you able to add a changelog and a testcase?

@dwoz dwoz added this to the Argon v3008.0 milestone Dec 12, 2023
@Obbi89
Copy link
Author

Obbi89 commented Feb 1, 2024

Ah well, I did not get notified. I quickly took a look and tried to get something basic running

@dwoz You might have to point me to some examples to get this done right.
tbh. I'm not sure how to test this.

My approach now is:

  • create a folder,
  • chmod it to ugo-w
  • use it as CWD for salt-call state.single file.managed check_cmd=/bin/true contents=['foobar']
    • (which will fail since cwd is not writeable. With my one-line fix however this state succeeds.)

To me this feels a bit nonsensical. It 'works' but does it actually test anything?

@Obbi89 Obbi89 force-pushed the master branch 3 times, most recently from a6a2487 to 37dbb06 Compare February 1, 2024 23:33
@Obbi89
Copy link
Author

Obbi89 commented Nov 11, 2024

duplicate #66098
fixed by #66099

@Obbi89 Obbi89 closed this Nov 11, 2024
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.

[BUG] file.managed check_cmd places tempfiles in filesystem root
2 participants