Skip to content

[master] Fixed issue with add_store and del_store - #64659

Open
HarshalSawant1234 wants to merge 3 commits into
saltstack:masterfrom
HarshalSawant1234:master
Open

[master] Fixed issue with add_store and del_store #64659
HarshalSawant1234 wants to merge 3 commits into
saltstack:masterfrom
HarshalSawant1234:master

Conversation

@HarshalSawant1234

@HarshalSawant1234 HarshalSawant1234 commented Jul 18, 2023

Copy link
Copy Markdown

Enable saltenv in add_store and del_store

What issues does this PR fix or reference?

Fixes:
Certificate not found error when running state with saltenv other than base

Previous Behavior

local:

      ID: security_deploy_certificates
Function: certutil.add_store
    Name: salt://base/files/certificates/wildcard.cer
  Result: False
 Comment: Certificate file not found.
 Started: 11:09:30.709032
Duration: 31.246 ms
 Changes:

Summary for local

Succeeded: 0
Failed: 1

Total states run: 1
Total run time: 31.246 ms

New Behavior

local:

      ID: security_deploy_itsb_certificates
Function: certutil.add_store
    Name: salt://base/files/certificates/wildcard.cer
  Result: True
 Comment:
 Started: 11:47:55.578772
Duration: 263.524 ms
 Changes:
          ----------
          added:
              salt://base/files/certificates/wildcard.cer

Summary for local

Succeeded: 1 (changed=1)
Failed: 0

Total states run: 1
Total run time: 263.524 ms

Merge requirements satisfied?

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

Commits signed with GPG?

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.

@HarshalSawant1234
HarshalSawant1234 requested a review from a team as a code owner July 18, 2023 06:35
@HarshalSawant1234
HarshalSawant1234 requested review from twangboy and removed request for a team July 18, 2023 06:35
@welcome

welcome Bot commented Jul 18, 2023

Copy link
Copy Markdown

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 Fixed issue with add_store and del_store [master] Fixed issue with add_store and del_store Jul 18, 2023
Ch3LL
Ch3LL suggested changes Jul 20, 2023
View reviewed changes

@Ch3LL Ch3LL 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 will also require a changelog and test coverage

Comment thread salt/modules/win_certutil.py Outdated


def del_store(source, store, retcode=False, saltenv="base"):
def del_store(source, store, saltenv="base", retcode=False):

@Ch3LL Ch3LL Jul 20, 2023

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.

Instead of changing the position of the kwargs, can we just call the kwarg from the call in the state. I'll include example in the state module below.

@twangboy twangboy Apr 2, 2025

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 ^^^

Comment thread salt/states/win_certutil.py Outdated
return ret

cert_serial = __salt__["certutil.get_cert_serial"](name)
cert_serial = __salt__["certutil.get_cert_serial"](name, saltenv)

@Ch3LL Ch3LL Jul 20, 2023

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.

Suggested change
cert_serial = __salt__["certutil.get_cert_serial"](name, saltenv)
cert_serial = __salt__["certutil.get_cert_serial"](name, saltenv=saltenv)

This would also apply to the other calls below

@Ch3LL Ch3LL added the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jul 20, 2023
@dwoz dwoz added this to the Argon v3008.0 milestone Dec 18, 2023
@dwoz
dwoz requested a review from a team as a code owner March 16, 2025 22:09
twangboy
twangboy requested changes Apr 2, 2025
View reviewed changes

@twangboy twangboy 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.

Please create a changelog and address comments form @Ch3LL

@twangboy twangboy added pending-changes The pull request needs additional changes before it can be merged needs-changelog labels Jul 2, 2025
@twangboy twangboy modified the milestones: Argon v3008.0, Argon v3008.1 Jun 5, 2026
HarshalSawant1234 and others added 3 commits June 15, 2026 18:36
@HarshalSawant1234 @dwoz
Add missing saltenv tag which was causing file not found error in the…
9b1bbda 
… module

State was only looking into the base saltenv for certificate file when add_store or del_store was called.
@HarshalSawant1234 @dwoz
Fixed module not loading when called from its respective state
02c0e56 
FIX: SyntaxError positional argument follows keyword argument
@dwoz
Address review feedback: use saltenv kwarg, fix del_store, add tests
11660b0 
- Revert the parameter-order swap in win_certutil module (keep
  retcode before saltenv to avoid breaking callers using positional
  arguments) per twangboy/Ch3LL review feedback
- Pass saltenv=saltenv as a keyword argument in all four state-to-module
  calls so non-base saltenvs are honoured during cert resolution
- Fix the previously-missed get_cert_serial call in del_store (it was
  still hardcoded to saltenv="base")
- Add regression tests verifying saltenv propagation in both
  add_store and del_store state functions
- Add changelog fragment (64659.fixed.md)
@dwoz dwoz modified the milestones: Argon v3008.1, Potassium v3009.0 Jun 16, 2026
@dwoz dwoz added test:full Run the full test suite and removed needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases needs-changelog pending-changes The pull request needs additional changes before it can be merged labels Jun 16, 2026
@twangboy

twangboy commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Please fix this on the earliest branch where the bug exists... probably 3006.x

This branch was previously deployed

1 inactive deployment
ci — 11660b01 Deployed Jun 19, 2026 by dwoz via Prepare Workflow Run #24225
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@twangboy twangboy twangboy approved these changes
+1 more reviewer
@Ch3LL Ch3LL Ch3LL requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test:full Run the full test suite

Projects

None yet

Milestone

Potassium v3009.0

Development

Successfully merging this pull request may close these issues.

4 participants

@HarshalSawant1234 @twangboy @Ch3LL @dwoz