-
Notifications
You must be signed in to change notification settings - Fork 80
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
Meshsync config on Meshery Operator CRD #533
Meshsync config on Meshery Operator CRD #533
Conversation
mod tidy Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
configs Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Layer5 Community Welcome Guide and sure to join the community Slack. |
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #533 +/- ##
==========================================
+ Coverage 49.70% 53.30% +3.59%
==========================================
Files 9 9
Lines 513 514 +1
==========================================
+ Hits 255 274 +19
+ Misses 236 221 -15
+ Partials 22 19 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
api/v1alpha1/meshsync_types_test.go
Outdated
@@ -51,6 +51,28 @@ var _ = Describe("The test case for the meshsync CRDs", func() { | |||
Name: "default", | |||
}, | |||
}, | |||
Config: MeshsyncConfig{ |
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.
Thanks for updating the tests cases
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.
Thanks for your contributing. However, there are many things need to be more specific. Please join the Dev meeting and talk to our activate maintainer @leecalcote @theBeginner86. Make sure we have a clearly outline for this feature. Thanks
api/v1alpha1/meshsync_types.go
Outdated
// MeshSyncSpec defines the desired state of MeshSync | ||
type MeshSyncSpec struct { | ||
Size int32 `json:"size,omitempty" yaml:"size,omitempty"` | ||
Broker MeshsyncBroker `json:"broker,omitempty" yaml:"broker,omitempty"` | ||
Config MeshsyncConfig `json:"config,omitempty" yaml:"config,omitempty"` |
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.
If we want to add a new field to track the configuration of Meshsync. It should show in the MeshSyncStatus
. We can set the configuration, but we cannot check it by checking the MeshSync status. It is not we what we want to.
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 ability to control resources to watch should be configurable from other than MeshSync CR.
What other ways have you thought of?
- ConfigMap
- Env
- ........
What should be the priority order for all of these?
Is configuration for tier discovery also considered?
Alternatively, can we move/augment the existing constructs for defining PipelineConfigs
?
i.e. Defining configs per MeshModel constructs (model, comps...)
This will also help in fingerprinting discovered resources into Meshery Models.
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
Signed-off-by: Daniel Kiptoon <[email protected]>
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.
Rest looks good.
api/v1alpha1/meshsync_types_test.go
Outdated
Data: map[string]string{ | ||
"blacklist": "blacklist", | ||
"global": "global-configs", | ||
"local": "global-configs", |
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.
"local": "global-configs", | |
"local": "local-configs", |
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.
@KiptoonKipkurui I think this is the reason why tests are failing.
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.
@MUzairS15 i have fixed the failing unit tests,made them more representative of how the configmap should look with blacklist and whitelisted resources
data: | ||
blacklist: "[\"namespaces.v1.\",\"configmaps.v1.\"]" | ||
whitelist: "[{\"Resource\":\"namespaces.v1.\",\"Events\":[\"ADDED\",\"DELETE\"]},{\"Resource\":\"replicasets.v1.apps\",\"Events\":[\"ADDED\",\"DELETE\"]},{\"Resource\":\"pods.v1.\",\"Events\":[\"MODIFIED\"]}]" |
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.
Please follow the same tense for all events. (ADDED, MODIFIED, DELETED OR ADD, MODIFY, DELETE)
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.
Let me supply the whole list of possible resources and events as a template that one can then modify as needed
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.
@MUzairS15 I have added to the sample config the list of default resources, for blacklist and whitelist resources
Add and reduce from the list accordingly Signed-off-by: Daniel Kiptoon <[email protected]>
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.
LGTM.
test more representative of the final config object Signed-off-by: Daniel Kiptoon <[email protected]>
Thank you for contributing to the Layer5 community! 🎉 \ \ |
Description
This PR adds Meshsync configuration information to Meshsync CRD
This PR fixes #532
Notes for Reviewers
Signed commits