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

[3006.x] Make sure the file client is destroyed upon used #64113

Merged
merged 5 commits into from
Apr 28, 2023
Merged

[3006.x] Make sure the file client is destroyed upon used #64113

merged 5 commits into from
Apr 28, 2023

Conversation

s0undt3ch
Copy link
Collaborator

@s0undt3ch s0undt3ch commented Apr 19, 2023 •
edited
Loading

What does this PR do?

See title

Fixes: #64111

@s0undt3ch s0undt3ch added the Sulfur v3006.0 release code name and version label Apr 19, 2023
@s0undt3ch s0undt3ch added this to the Sulfur v3006.1 milestone Apr 19, 2023
@s0undt3ch s0undt3ch requested a review from a team as a code owner April 19, 2023 16:47
@s0undt3ch s0undt3ch requested review from Ch3LL and removed request for a team April 19, 2023 16:47
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Make sure the file client is destroyed upon used [3006.x] Make sure the file client is destroyed upon used Apr 19, 2023
@s0undt3ch s0undt3ch temporarily deployed to ci April 19, 2023 17:46 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 19, 2023 17:46 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 19, 2023 17:51 — with GitHub Actions Inactive
@anilsil anilsil added Sulfur v3006.1 and removed Sulfur v3006.0 release code name and version labels Apr 19, 2023
@s0undt3ch s0undt3ch temporarily deployed to ci April 19, 2023 20:16 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 19, 2023 20:16 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 19, 2023 20:18 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 19, 2023 21:18 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 19, 2023 21:18 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 19, 2023 21:18 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 19, 2023 23:15 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 19, 2023 23:15 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 19, 2023 23:15 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 24, 2023 17:01 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 24, 2023 17:01 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 24, 2023 17:01 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch mentioned this pull request Apr 24, 2023
Closed
@s0undt3ch s0undt3ch temporarily deployed to ci April 24, 2023 18:15 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 24, 2023 18:15 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 24, 2023 18:40 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch requested a review from dwoz April 25, 2023 11:20
dwoz
dwoz requested changes Apr 25, 2023
View reviewed changes
Copy link
Contributor

@dwoz dwoz left a comment

Choose a reason for hiding this comment

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

Can we adapt this test to use destroy instead of removing it? Basically, test the case when we do and do not pass a file_client to SaltCacheLoader.

@s0undt3ch
Copy link
Collaborator Author

Can we adapt this test to use destroy instead of removing it? Basically, test the case when we do and do not pass a file_client to SaltCacheLoader.

You mean the test that I removed? Or the one you added?

@dwoz
Copy link
Contributor

dwoz commented Apr 25, 2023

Can we adapt this test to use destroy instead of removing it? Basically, test the case when we do and do not pass a file_client to SaltCacheLoader.

You mean the test that I removed? Or the one you added?

The one getting removed. I think there is value in testing what happens when there is and is not a file client passed to SaltCacheLoader.

@s0undt3ch s0undt3ch requested review from dwoz and twangboy April 26, 2023 18:26
@s0undt3ch
Copy link
Collaborator Author

Can we adapt this test to use destroy instead of removing it? Basically, test the case when we do and do not pass a file_client to SaltCacheLoader.

You mean the test that I removed? Or the one you added?

The one getting removed. I think there is value in testing what happens when there is and is not a file client passed to SaltCacheLoader.

Unit test added.

@s0undt3ch s0undt3ch temporarily deployed to ci April 26, 2023 21:40 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 26, 2023 21:41 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 26, 2023 21:41 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 27, 2023 00:01 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 27, 2023 00:01 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch temporarily deployed to ci April 27, 2023 00:01 — with GitHub Actions Inactive
@s0undt3ch s0undt3ch linked an issue Apr 27, 2023 that may be closed by this pull request
[BUG] [3006.0] Regression: Include sls that includes jinja macros leads to Rendering SLS error #64111
Closed
2 tasks
dwoz
dwoz approved these changes Apr 27, 2023
View reviewed changes
Copy link
Contributor

@dwoz dwoz left a comment

Choose a reason for hiding this comment

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

Approved, though I'd like the destroy method to be simplified if possible.

salt/utils/jinja.py Show resolved Hide resolved
@s0undt3ch s0undt3ch merged commit 2e3f98a into saltstack:3006.x Apr 28, 2023
@s0undt3ch s0undt3ch deleted the hotfix/golden-images branch April 28, 2023 05:24
s0undt3ch added a commit to s0undt3ch/salt that referenced this pull request Apr 28, 2023
@s0undt3ch
Add missing changelog entries for the work done in saltstack#64113
5c9b394 
Signed-off-by: Pedro Algarvio <[email protected]>
@s0undt3ch s0undt3ch mentioned this pull request Apr 28, 2023
Merged
s0undt3ch added a commit to s0undt3ch/salt that referenced this pull request Apr 28, 2023
@s0undt3ch
Add missing changelog entries for the work done in saltstack#64113
ffb015f 
Signed-off-by: Pedro Algarvio <[email protected]>
agraul added a commit to openSUSE/salt that referenced this pull request Apr 28, 2023
@agraul
Make sure the file client is destroyed upon used
a1fc528 
Backport of saltstack/salt#64113
Ch3LL pushed a commit that referenced this pull request Apr 28, 2023
@s0undt3ch
Add missing changelog entries for the work done in #64113
dbc2158 
Signed-off-by: Pedro Algarvio <[email protected]>
@cianyleow cianyleow mentioned this pull request Oct 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwoz dwoz dwoz approved these changes

@twangboy twangboy twangboy approved these changes

@Ch3LL Ch3LL Awaiting requested review from Ch3LL Ch3LL was automatically assigned from saltstack/team-core

Assignees
No one assigned
Labels
Sulfur v3006.1
Projects
None yet
Milestone
Sulfur v3006.1
Development

Successfully merging this pull request may close these issues.

[BUG] [3006.0] Regression: Include sls that includes jinja macros leads to Rendering SLS error
4 participants
@s0undt3ch @dwoz @twangboy @anilsil