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

[ADDED] system model command #8284

Merged
merged 43 commits into from Nov 22, 2023
Merged

Conversation

zakisk
Copy link
Contributor

@zakisk zakisk commented Jul 26, 2023

Signed-off-by: Mohammed Zaki zs84907@gmail.com

Notes for Reviewers

This PR fixes #8176

Signed commits

  • Yes, I signed my commits.
  • Yes, I added unit tests.

@github-actions
Copy link

github-actions bot commented Jul 26, 2023

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2949e28) 10.11% compared to head (f1f2c41) 0.00%.
Report is 63 commits behind head on master.

❗ Current head f1f2c41 differs from pull request most recent head b3afb62. Consider uploading reports for the commit b3afb62 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #8284       +/-   ##
==========================================
- Coverage   10.11%       0   -10.12%     
==========================================
  Files         136       0      -136     
  Lines       21193       0    -21193     
==========================================
- Hits         2143       0     -2143     
+ Misses      18729       0    -18729     
+ Partials      321       0      -321     
Flag Coverage Δ
unittests ?

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.

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.

@zakisk will you please see that each error code has a probable cause and a suggested remediation defined?

@zakisk
Copy link
Contributor Author

zakisk commented Jul 27, 2023

@abdullah1308 as you can see in this PR that I've deleted mesheryctl/internal/cli/root/error.go file because I wanted to refernce them in mesheryctl/internal/cli/root/system package I was getting import cycle error and I was unable to reference them, and error functions in mesheryctl/internal/cli/root/error.go were only used by mesheryctl/internal/cli/root/version.go file so I copied all of them in mesheryctl/internal/cli/root/system/error.go file and chaged mesheryctl/internal/cli/root/version.go file.

@abdullah1308
Copy link
Contributor

@zakisk The approach that me, @Philip-21 and @suhail34 have decided on is to move errors common to all mesheryctl commands to mesheryctl/pkg/utils/error.go. For errors that are specific to a sub-command they will be stored in the error.go file of sub-command's package.

There's 1/2 PRs up for making this change already. For now I suggest that you undo the change related to deleting the error.go file and moving the error codes. I understand that this causes the import cycle error. However once these other PRs are closed and if master is merged into this PR, these errors should be resolved. Hope this makes sense.

@zakisk
Copy link
Contributor Author

zakisk commented Jul 27, 2023

@abdullah1308 error file that I've deleted was used by only one other file i.e version.go so I don't think that it is worth to undo deletion of that file. but if you still suggest me to do so I will add that error file in one more commit to this PR, is that you want?

@zakisk
Copy link
Contributor Author

zakisk commented Jul 27, 2023

@abdullah1308 the approach I'm thinking of is that after your PR are merged we can remove all generic errors from mesheryctl in a new PR, does this sound good?

@abdullah1308
Copy link
Contributor

@abdullah1308 error file that I've deleted was used by only one other file i.e version.go so I don't think that it is worth to undo deletion of that file. but if you still suggest me to do so I will add that error file in one more commit to this PR, is that you want?

@zakisk Yeah that is what I'd suggest for now.

@alphaX86
Copy link
Member

alphaX86 commented Sep 8, 2023

@Freedisch the command you've suggested is adding new commit with the same message of old commit of which SHA key is passed it is not amending it on place.

If that's the case, then ignore the DCO for now. We'll take care of that later

@@ -0,0 +1,108 @@
CATEGORY MODEL VERSION
Copy link
Member

Choose a reason for hiding this comment

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

You might need explanation for doing this here as this will be dynamic and change from time to time

@zakisk
Copy link
Contributor Author

zakisk commented Sep 8, 2023

@alphaX86 API response is stored in golden file and the output will be same every time when tests run, in order to keep uniformity in tests pattern I did this because all other tests you can check in fliter or other commands are following this pattern,

@alphaX86
Copy link
Member

alphaX86 commented Sep 8, 2023

@alphaX86 API response is stored in golden file and the output will be same every time when tests run, in order to keep uniformity in tests pattern I did this because all other tests you can check in fliter or other commands are following this pattern,

OK, you indeed need a token for them. I understand the requirement. But did you hardcode it?

@zakisk
Copy link
Contributor Author

zakisk commented Sep 8, 2023

@alphaX86 yes

@alphaX86
Copy link
Member

alphaX86 commented Sep 9, 2023

@alphaX86 yes

Is this token related to your Meshery Cloud account?

@zakisk
Copy link
Contributor Author

zakisk commented Sep 9, 2023

@alphaX86 I've copied this from filter test its not mine.

@zakisk
Copy link
Contributor Author

zakisk commented Sep 9, 2023

@alphaX86 and its more than two years old token.

@alphaX86
Copy link
Member

alphaX86 commented Sep 9, 2023

@alphaX86 I've copied this from filter test its not mine.

Ok, noted 👍

@zakisk
Copy link
Contributor Author

zakisk commented Sep 9, 2023

@alphaX86 @suhail34 @leecalcote @MUzairS15 is there anything else?

@zakisk
Copy link
Contributor Author

zakisk commented Sep 15, 2023

@leecalcote @alphaX86 ??

@alphaX86
Copy link
Member

@leecalcote @alphaX86 ??

I'll review this once I'm in my system

@zakisk
Copy link
Contributor Author

zakisk commented Sep 20, 2023

@leecalcote @alphaX86 ??

I'll review this once I'm in my system

@alphaX86 have you done this?

@zakisk
Copy link
Contributor Author

zakisk commented Oct 21, 2023

@leecalcote @alphaX86 it's been a while this PR is still open.

@alphaX86 alphaX86 requested a review from a team October 21, 2023 12:34
@leecalcote
Copy link
Member

@MUzairS15, please review.

@leecalcote leecalcote requested review from aboobakersiddiqr63, leecalcote and suhail34 and removed request for Boombag0607 and leecalcote October 30, 2023 18:00
@leecalcote
Copy link
Member

@zakisk great demo today. Thanks for taking everyone through your work. We’re all green 🚦 here.

@leecalcote leecalcote merged commit eeb83c2 into meshery:master Nov 22, 2023
15 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/mesheryctl CLI for Meshery component/server issue/dco Commit signoff instructions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mesheryctl] Add mesheryctl system model
10 participants