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

Refactor and move Patterns constructs. #376

Closed
wants to merge 6 commits into from

Conversation

MUzairS15
Copy link
Contributor

Description

This PR

  1. Refactors and moves Patterns constructs into meshkit, to ensure any component can use it.
    This will also be helpful when we incorporate WASM, as some Pattern constructs would be required and importing the meshery server as a package is not ideal.
  2. Adds description and icon attributes as HelmMetadata.
    Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

Signed-off-by: MUzairS15 <[email protected]>
}

func ErrPatternFromCytoscape(err error) error {
return errors.New(ErrPatternFromCytoscapeCode, errors.Alert, []string{"Could not create design file from given cytoscape"}, []string{err.Error()}, []string{"Invalid cytoscape body", "Service name is empty for one or more services", "_data does not have correct data"}, []string{"Make sure cytoscape is valid", "Check if valid service name was passed in the request", "Make sure _data field has \"settings\" field"})
Copy link
Member

Choose a reason for hiding this comment

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

@MUzairS15 let's not speak to users in terms of the tech that we use. Please put on your user hat and re-write.

res := convertMapInterfaceMapString(in)
out, ok := res.(map[string]interface{})
if !ok {
fmt.Println("failed to cast")
Copy link
Member

Choose a reason for hiding this comment

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

We don't use Println...

return res
}

func GetRandomAlphabetsOfDigit(length int) (s string) {
Copy link
Member

Choose a reason for hiding this comment

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

Function description needed.

@@ -245,3 +245,19 @@ func Contains[G []K, K comparable](slice G, ele K) bool {
}
return false
}

func ManifestIsEmpty(manifests []string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Please add function description.

}
return true
}

Copy link
Member

Choose a reason for hiding this comment

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

Please add function description.

@@ -19,7 +19,6 @@ func (e *Event) BeforeUpdate(tc *gorm.DB) (err error) {
}

func isEventStatusSupported(event *Event) error {

if event.Status != Read && event.Status != Unread {
Copy link
Member

Choose a reason for hiding this comment

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

Should this reference a struct/iota/enum from the Eventv2 schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is referencing, if you see the schema Status is defined as an enum

AppVersion string `yaml:"appVersion"`
Name string `yaml:"name"`
Version string `yaml:"version"`
APIVersion string `yaml:"apiVersion"`
Copy link
Member

Choose a reason for hiding this comment

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

Is there anyway for MeshKit to reference the Helm Controller model, using values ComponentDefinitions (granted that we wouldn't support all types of Helm connections immediately)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This struct represents the index.yml and is used when deploying/fetching the helm charts. Referencing the Helm Controller model wouldn't serve the purpose here.

Copy link
Member

Choose a reason for hiding this comment

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

It would serve that purpose, but also many other purposes. I'm asking if we can reference that component, and only expose a single field.

}

func ErrCreatePatternService(err error) error {
return errors.New(ErrCreatePatternServiceCode, errors.Alert, []string{"Failed to create design service from Manifest"}, []string{err.Error()}, []string{"Invalid Manifest", "Meshery doesn't identifies the Resource mentioned in the Manifest"}, []string{"Check if all of the meshery adapters are running", "Check if Meshery has successfully identified and registered Kubernetes components"})
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize Meshery Adapters.

What type of Manifests are these? Kubernetes? If so, write "Kubernetes manifests".

}

// Service represents the services defined within the appfile
type Service struct {
Copy link
Member

Choose a reason for hiding this comment

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

Outside of backwards compatibility and other changes that would be needed, there is no longer a need for Service, is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no longer a need for Service

Yes.
The pattern engine will also require changes.

@@ -0,0 +1,698 @@
package patterns
Copy link
Member

Choose a reason for hiding this comment

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

Where is the OpenAPI schema for this resource? Let's ensure that this package, structs, functions are based directly on that schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Below is the spec for Meshery Patterns, the structs defined here are internal (except DryRun).
https://github.com/meshery/meshery/blob/db401c30f56ff47eae6849bf19aad0c32d8dac2a/server/helpers/swagger.yaml#L2036

Copy link
Member

Choose a reason for hiding this comment

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

Let's put this PR on hold until the spec is properly defined.

Copy link

stale bot commented Nov 14, 2023

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.

@stale stale bot added the issue/stale Issue has not had any activity for an extended period of time label Nov 14, 2023
Copy link

stale bot commented Nov 24, 2023

This issue is being automatically closed due to inactivity. However, you may choose to reopen this issue.

@stale stale bot closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/patterns issue/stale Issue has not had any activity for an extended period of time pr/on hold PR/Issue on hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants