-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Update contributing-model-relationships.md #11377
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Nithish Karthik <75248557+humblenginr@users.noreply.github.com>
🚀 Preview for commit 5ebf37e at: https://6690136ce471f914b33fb6c4--meshery-docs-preview.netlify.app |
@@ -330,6 +329,8 @@ Each policy has a set of evaluation rules defined and the `evaluationQuery` attr | |||
1. To configure a relationship to be applied across all versions of a particular model, ensure the `version` property for those relationships is set to `*`, to limit the relationships to a specific version of a model, specify the correct model version. | |||
1. Specify `version` property as a regex to ensure relationships are applied to a subset of versions of a model. | |||
|
|||
{% include alert.html title="Caution While Defining Relationships" type="warning" content="Relationships are not imported directly and they have to be packaged inside a model. Use caution when defining relationships that span models." %} |
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.
{% include alert.html title="Caution While Defining Relationships" type="warning" content="Relationships are not imported directly and they have to be packaged inside a model. Use caution when defining relationships that span models." %} | |
{% include alert.html title="Gratuitous Scoping of Relationships" type="warning" content="Relationships are not imported directly and they have to be packaged inside a model. Use caution when defining relationships that span models." %} |
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.
"Relationships are not imported directly and they have to be packaged inside a model." There is some relevance of this information to the warning, but it's not clear what that is given this phrasing and lack of explanation. Please revisit.
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.
"Use caution when defining relationships that span models." Great. Why? Where's the explanation here?
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.
Reviewing again...
This feedback will need to be incorporated prior to merge.
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.
Will update it
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.
This feedback has not been addressed.
@@ -330,6 +329,8 @@ Each policy has a set of evaluation rules defined and the `evaluationQuery` attr | |||
1. To configure a relationship to be applied across all versions of a particular model, ensure the `version` property for those relationships is set to `*`, to limit the relationships to a specific version of a model, specify the correct model version. | |||
1. Specify `version` property as a regex to ensure relationships are applied to a subset of versions of a model. | |||
|
|||
{% include alert.html title="Caution While Defining Relationships" type="warning" content="Relationships are not imported directly and they have to be packaged inside a model. Use caution when defining relationships that span models." %} |
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.
"Relationships are not imported directly and they have to be packaged inside a model." There is some relevance of this information to the warning, but it's not clear what that is given this phrasing and lack of explanation. Please revisit.
@@ -330,6 +329,8 @@ Each policy has a set of evaluation rules defined and the `evaluationQuery` attr | |||
1. To configure a relationship to be applied across all versions of a particular model, ensure the `version` property for those relationships is set to `*`, to limit the relationships to a specific version of a model, specify the correct model version. | |||
1. Specify `version` property as a regex to ensure relationships are applied to a subset of versions of a model. | |||
|
|||
{% include alert.html title="Caution While Defining Relationships" type="warning" content="Relationships are not imported directly and they have to be packaged inside a model. Use caution when defining relationships that span models." %} |
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.
"Use caution when defining relationships that span models." Great. Why? Where's the explanation here?
@humblenginr Could we discuss this issue during the Meshery dev meeting call? Please add it as an agenda item to the meeting minutes. |
@humblenginr, thoughts on the feedback offered? Please accept or reject each point of feedback. |
Co-authored-by: Lee Calcote <leecalcote@gmail.com> Signed-off-by: Nithish Karthik <75248557+humblenginr@users.noreply.github.com>
🚀 Preview for commit ba04c00 at: https://669a5eabb2a59436b89e90e3--meshery-docs-preview.netlify.app |
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.
The current warning is insufficiently introduced to the reader, leaving them no better off in terms of the objective of this warning.
Signed-off-by: Nithish Karthik <nithishkarthik01@gmail.com>
Updated the warning |
🚀 Preview for commit 0ad4ecc at: https://669b21d028818da1854f9ebe--meshery-docs-preview.netlify.app |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Notes for Reviewers
This PR fixes #
Signed commits