Res validation should be aware of action prop reqs.

[sabat]

Signed-off-by: Steve Abatangle [email protected]

Description

(Lamont has seen this patch before. It was never merged because of some confusion in the conversation, about a year ago. The bug remains in the most current Infra release.)

The get and set methods in the Property class (for resource validation) check that a property is required if its value is nil. However, they don't check to see whether the property was required for the specific action the resource is using, so any use of get and set will throw an exception if used with a resource that doesn't use a required property, even when the property is not required for that action.

What I'm describing is easier to understand with an example: the current version of the chef-client cookbook has a recipe (cron.rb) with a resource that uses the :delete action. When you try to get that resource, it throws an exception, explaining that :command is a required property—but it's only required for the :create action, not for :delete.

See https://github.com/chef-cookbooks/chef-client/blob/master/recipes/cron.rb#L95-L97

This PR fixes that by passing the resource's action to the required? method.

I have included a unit test update. I would also have included an integration test, but that seems to require deeper knowledge about Chef plumbing than I have.

Related Issue

Types of changes

• [X] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

• [X] I have read the CONTRIBUTING document.
• [X] I have run the pre-merge tests locally and they pass.
• [ ] I have updated the documentation accordingly.
• [X] I have added tests to cover my changes.
• [X] All new and existing tests passed.
• [X] All commits have been signed-off for the Developer Certificate of Origin.

[jaymzh]

I don't think the test failures are related, but we should get tests working before we merge this so we can see if this breaks anything - @marcparadise and @johnmccrae

Also, @sabat - you have lint failuers.

Other than that seems good.

[sabat]

@jaymzh pushed a fix to that lint error—just a trailing space so easy to fix

[jaymzh]

Thanks!

Approved tests, lets see if we're in a better state

[jaymzh]

@sabat thanks! Can you rebase, all the test failures you are hitting have been fixed. Sorry!

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
5.0% Duplication

[sabat]

@jaymzh rebased, pushed, build still failing in two Windows steps unrelated to this PR.

[tpowell-progress]

Ready to merge but holding off until we have Chef 18 RC packaged.

[tpowell-progress]

@sabat Now that GA has been released, can you please rebase so that we can confirm that tests still pass with these changes?

[johnmccrae]

@sabat one minor request - would you mind squashing your commits here to clean this up?

[johnmccrae]

I'll merge it after that.

[tpowell-progress]

@sabat Looks like you need to squash and rebase and verify all the commits have the DCO signed. Once that it done, we'll need to make sure the pipelines are still green for this PR.

[sabat]

Looks like you need to squash and rebase and verify all the commits have the DCO signed Didn't I literally just do that? For a customer trying to help fix a flaw in your product, this back-and-forth is getting frustrating.

On Tue, Nov 15, 2022 at 12:31 PM tpowell-progress @.***> wrote:

@sabat https://github.com/sabat Looks like you need to squash and rebase and verify all the commits have the DCO signed. Once that it done, we'll need to make sure the pipelines are still green for this PR.

— Reply to this email directly, view it on GitHub https://github.com/chef/chef/pull/13069#issuecomment-1315830074, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAQYHF6ULMZ4WSX4HUTCOTWIPXLTANCNFSM53IUKZAA . You are receiving this because you were mentioned.Message ID: @.***>

[ramereth]

it looks like you had a lot of merged updates that caused your history to get all confusing. I went through and rebased it the best I could and pushed it here as one commit: https://github.com/ramereth/chef/tree/bugfix_rsrc_action_prop_validation

Let me know if that diff looks correct with all the fixes you expect.

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
4.6% Duplication