-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
2 enhancements to config.option #51677
Conversation
+1 This will solve my original issue! Thanks Erik. |
213adc8
to
fe31e35
Compare
It looks like Jenkins was in a bad state when I opened this, so since it's been a while I rebased and pushed to trigger new tests. |
@Ch3LL I opened this 4 months ago and had added information to the Neon release notes, assuming that this would have been merged before we branched for Neon. Since that never happened, please let me know if you would like to either A) re-point this PR at Neon (will require a rebase from me, which I'm happy to do), or B) have me modify this PR to add the release notes to Sodium. |
doc/topics/releases/neon.rst
Outdated
module now supports perstant changes with ``persist=True`` by calling the | ||
:py:func:`selinux.fcontext_add_policy <salt.modules.selinux.fcontext_add_policy>` module. | ||
- The :py:func:`file.set_selinux_context | ||
<salt.modules.file.set_selinux_context>` module now supports perstant changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/persistant/persistent
matches is the same as in :py:func:`config.get <salt.modules.config.get>`, | ||
only this function does not recurse into nested data structures. Another | ||
difference between this function and :py:func:`config.get | ||
<salt.modules.config.get>` is that it comes with a set of "sane defaults". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would "reasonable defaults" read better here?
ping @s0undt3ch or @twangboy any ideas why the windows tests are not finishing here? |
@Ch3LL I think it might be the changes we got merged in yesterday. @terminalmage you mind rebasing on neon, I believe that will fix the issue 🤞 |
These now reside in salt.modules.config.DEFAULTS
Given that the tests weren't even starting, and I didn't modify any files that affect Windows, I'm not sure I follow your logic. But I rebased again anyway, and sure enough, tests aren't even starting on Windows:
By the way, you can see Jenkins' console output by clicking the "Console" link on the Jenkins test results. |
I've had it happen where I'm trying to merge against say commit A on branch develop, and the build fails. Commits happen on develop and now we're at commit C. Rebasing (or merging C into my branch) triggers a rebuild, which will fix whatever. I'm not sure why GitHub says that a branch is out of date with the original... that seems pretty weird, now that you point it out Regardless, it looks like now the only problem is this known Windows failure (Pedro is looking into it right now). Hopefully that shouldn't take too much. |
Again, the console was pretty clear that the failure was in just starting the tests. There is actionable information that we can use, so I am honestly confused (and a little dismayed) that the plan seems to be to cross our fingers that "whatever" will be fixed by pushing and triggering another build.
Really? You can see this change in the GitHub web interface when a pull request is merged into a branch. All the other PRs which were opened against that same branch are now out-of-date with the base branch since the base branch has commits that the head branch does not. |
Ah, I had a fuzzy mental model going on, never mind 😝
Apologies - I didn't actually check the console because I had been noticing failed windows builds for the last few days, and I knew that a couple of commits had gone in to fix other windows failures. I made an (incorrect) educated guess that this windows failure was related to the other failures. It wasn't until today that I learned of this particular class of failures. This was bad pattern matching on my part. Probably also driven by the fact that PRs have to be brought up-to-date before merging. Next time I'll spend the time to ensure that the build failures are what I expect them to be. |
looks like the tests are passing. @waynew are we just waiting on your re-review? |
This makes the following two changes to
config.option
:For backward compatibility, wildcard matches are only attempted when
wildcard=True
is passed.Additionally, the Docker registry configuration now uses
config.option
, meaning that registry auth can be configured elsewhere than the pillar data.Resolves #51531.