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

[mesheryctl] Removal of the onboard Sub-command from Mesheryctl Pattern Command #10929

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

singh1203
Copy link
Contributor

Notes for Reviewers

This PR fixes #10886

Signed commits

  • Yes, I signed my commits.

@github-actions github-actions bot added the component/mesheryctl CLI for Meshery label May 11, 2024
Copy link

github-actions bot commented May 11, 2024

@singh1203 singh1203 marked this pull request as ready for review May 11, 2024 15:24
Copy link
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

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

@singh1203 will you list the various test scenarios that you've tried and their results?

@singh1203
Copy link
Contributor Author

@singh1203 will you list the various test scenarios that you've tried and their results?

Currently haven't tested various scenarios, but will prioritize doing so ASAP and exploring different tests I can run.

@leecalcote
Copy link
Member

Any scenarios and results to report, @singh1203?

@singh1203
Copy link
Contributor Author

singh1203 commented May 17, 2024

Any scenarios and results to report, @singh1203?

Certainly! Here is a list of the various test scenarios and their results for the mesheryctl pattern command:

mesheryctl pattern apply

  • Applying with ID

    • Result: Failed
    • Issue: No pattern found with the given name or ID.
    • Error Message: "Fetching patterns... No Pattern found with the given name or ID."
  • Applying with file path

    • Result: Successful
    • Issue: Pattern is created but not deployed.
    • Error Message: "Applying pattern Simple Kubernetes Pod. Response Status Code 500, possibly Server error."
  • Documentation issues

    • Issue: Missing proper documentation for the command syntax (e.g., mesheryctl pattern apply [patternID]).

mesheryctl pattern import

mesheryctl pattern delete

  • Deleting with patternID

    • Result: Successful
    • Issue: None
  • Deleting with file path

    • Result: Failed
    • Issue: Response Status Code 500, possibly Server error.
    • Error Message: "Unable to create a response from request. Response Status Code 500, possibly Server error."
  • Deleting with URL

    • Result: Failed
    • Issue: Unclear what is meant by URL in the command syntax (mesheryctl pattern delete {URL to patternfile}).

mesheryctl pattern offboard

  • URL-based pattern deletion

    • Result: Not implemented
    • Issue: Lack of URL-based pattern deletion in the implementation.
  • Redundancy with delete sub-command

    • Issue: There is no significant difference between the offboard and delete sub-commands, suggesting the offboard sub-command could be removed.

mesheryctl pattern view

  • Viewing patterns
    • Result: Successful
    • Issue: Sometimes the response is too large to be fully viewed in the CLI.

mesheryctl pattern list

  • Listing patterns
    • Result: Successful
    • Issue: None

@leecalcote
Copy link
Member

@singh1203, good progress. Send word when the tests are passing.

Copy link

codecov bot commented Jun 2, 2024

Codecov Report

Attention: Patch coverage is 2.85714% with 34 lines in your changes missing coverage. Please review.

Project coverage is 8.86%. Comparing base (0bbdd7f) to head (78e1a56).
Report is 186 commits behind head on master.

Current head 78e1a56 differs from pull request most recent head 21c7d22

Please upload reports for the commit 21c7d22 to get more accurate results.

Files Patch % Lines
mesheryctl/internal/cli/root/pattern/import.go 0.00% 34 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10929      +/-   ##
==========================================
- Coverage    9.11%    8.86%   -0.25%     
==========================================
  Files         146      145       -1     
  Lines       19303    19144     -159     
==========================================
- Hits         1760     1698      -62     
+ Misses      17241    17163      -78     
+ Partials      302      283      -19     
Flag Coverage Δ
unittests 8.86% <2.85%> (-0.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@singh1203
Copy link
Contributor Author

singh1203 commented Jun 4, 2024

@leecalcote Implementing design pattern with pattern subcommands, but encountering server error (500) every time. I've Updated scenario condition above, but error persists.
Here is the issue #11110 for it.

@leecalcote
Copy link
Member

@leecalcote Implementing design pattern with pattern subcommands, but encountering server error (500) every time. I've Updated scenario condition above, but error persists. Here is the issue #11110 for it.

Will you confirm that you can successfully deploy this pattern via Meshery UI?

@leecalcote
Copy link
Member

If not, trying using a simple design that you are confident has no deployment issues.

@singh1203
Copy link
Contributor Author

@leecalcote Implementing design pattern with pattern subcommands, but encountering server error (500) every time. I've Updated scenario condition above, but error persists. Here is the issue #11110 for it.

Will you confirm that you can successfully deploy this pattern via Meshery UI?

Pattern_clip.mp4

I hope this clip addresses your question!

@singh1203
Copy link
Contributor Author

@lekaf974 Surely your eyes can see imperfections here.

Copy link
Contributor

@lekaf974 lekaf974 left a comment

Choose a reason for hiding this comment

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

Missing documentation update in docs/_data/mesheryctlcommands/cmds.yml

mesheryctl/internal/cli/root/pattern/apply.go Outdated Show resolved Hide resolved
@lekaf974
Copy link
Contributor

Example in --help needs to be updated

~ mesheryctl]$ ./mesheryctl pattern delete --help
delete pattern file will trigger deletion of the pattern file

Usage:
  mesheryctl pattern delete [flags]

Examples:

// delete a pattern file
mesheryctl pattern delete [file | URL]
	

Flags:
  -f, --file string   Path to pattern file
  -h, --help          help for delete

Global Flags:
      --config string   path to config file (default "/home/matthieu/.meshery/config.yaml")
  -t, --token string    Path to token file default from current context
  -v, --verbose         verbose output

@lekaf974
Copy link
Contributor

Looks Argurment validation is missing on delete sub-command

[~ mesheryctl]$ ./mesheryctl pattern delete
Access to this resource is unauthorized.
Access to this resource is unauthorized.
delete without any args/flags is making 2 calls to the backend

Could add a check to validate name args or --file flag is given following this example

Args: func(_ *cobra.Command, args []string) error {

@github-actions github-actions bot added the area/docs Documentation update needed label Jun 16, 2024
@l5io
Copy link
Collaborator

l5io commented Jun 16, 2024

}
return nil
},
Args: cobra.MinimumNArgs(0),
Copy link
Contributor

@lekaf974 lekaf974 Jun 17, 2024

Choose a reason for hiding this comment

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

Should use same implementation for Args that you made in the delete sub command for UX consistency

@l5io
Copy link
Collaborator

l5io commented Jun 19, 2024

@leecalcote
Copy link
Member

@singh1203 all comments resolved?

@l5io
Copy link
Collaborator

l5io commented Jun 29, 2024

@singh1203
Copy link
Contributor Author

@singh1203 all comments resolved?

Yes, almost all of it is now working regarding your suggestion of confirming a particular environment before applying and deleting any pattern(design).

Signed-off-by: Saurabh Kumar Singh <singh1203.ss@gmail.com>
Signed-off-by: Saurabh Kumar Singh <singh1203.ss@gmail.com>
Signed-off-by: Saurabh Kumar Singh <singh1203.ss@gmail.com>
Signed-off-by: Saurabh Kumar Singh <singh1203.ss@gmail.com>
Signed-off-by: Saurabh Kumar Singh <singh1203.ss@gmail.com>
Signed-off-by: Saurabh Kumar Singh <singh1203.ss@gmail.com>
Signed-off-by: Saurabh Kumar Singh <singh1203.ss@gmail.com>
@l5io
Copy link
Collaborator

l5io commented Jun 30, 2024

@leecalcote leecalcote requested a review from Jougan-0 July 1, 2024 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Documentation update needed component/mesheryctl CLI for Meshery
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mesheryctl] Remove onboard command from mesheryctl pattern commands
4 participants