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

Handle key collision with duplicate definitions using same parameters #1716

Merged
merged 11 commits into from
Oct 1, 2022

Conversation

ArmaanMcleod
Copy link
Contributor

@ArmaanMcleod ArmaanMcleod commented Sep 25, 2022

PR Summary

Related to #1653.

Fixes key collisions with definitions using same parameters.
Also refactored a bit of csharp code.

PR Checklist

  • PR has a meaningful title
  • Summarized changes
  • Change is not breaking
  • This PR is ready to merge and is not Work in Progress
  • Rule changes
    • Unit tests created/ updated
    • Rule documentation created/ updated
    • Link to a filed issue
    • Change log has been updated with change under unreleased section
  • Other code changes
    • Unit tests created/ updated
    • Link to a filed issue
    • Change log has been updated with change under unreleased section

@ArmaanMcleod ArmaanMcleod requested a review from a team as a code owner September 25, 2022 11:49
@ghost
Copy link

ghost commented Sep 25, 2022

CLA assistant check
All CLA requirements met.

@ArmaanMcleod
Copy link
Contributor Author

@BernieWhite Made some changes to only store definition parameters, let me know what you think.

Copy link
Collaborator

@BernieWhite BernieWhite left a comment

Choose a reason for hiding this comment

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

@ArmaanMcleod Nice work. I think functionally let's go with this. I think we can optimize this further over the next few releases, however I don't think we need to invest that time now as the code based is rapidly changing.

Let's fix these id's and after that we should be good to merge.

tests/PSRule.Rules.Azure.Tests/test11.assignment.json Outdated Show resolved Hide resolved
tests/PSRule.Rules.Azure.Tests/test11.assignment.json Outdated Show resolved Hide resolved
tests/PSRule.Rules.Azure.Tests/test11.assignment.json Outdated Show resolved Hide resolved
tests/PSRule.Rules.Azure.Tests/test11.assignment.json Outdated Show resolved Hide resolved
tests/PSRule.Rules.Azure.Tests/emittedJsonRulesData.jsonc Outdated Show resolved Hide resolved
@ArmaanMcleod
Copy link
Contributor Author

ArmaanMcleod commented Sep 30, 2022

@ArmaanMcleod Nice work. I think functionally let's go with this. I think we can optimize this further over the next few releases, however I don't think we need to invest that time now as the code based is rapidly changing.

Let's fix these id's and after that we should be good to merge.

@BernieWhite Thanks mate, all good to keep iterating on this in the next few releases. Next, I can probably add the name and ref properties.

I've updated the code with your suggestions 🙂.

There also seem to be some pending workflow approvals that are needed for first-time contributors, probably has to be done again after I signed the CLA again 😄.

Copy link
Collaborator

@BernieWhite BernieWhite left a comment

Choose a reason for hiding this comment

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

Thanks @ArmaanMcleod all good to merge.

@BernieWhite BernieWhite merged commit 5d77be0 into Azure:main Oct 1, 2022
@ArmaanMcleod ArmaanMcleod deleted the fix-dup-policy-issue branch October 1, 2022 05:47
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.

2 participants