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] Make 'system reset' context aware #2265

Merged
merged 8 commits into from Jan 22, 2021
Merged

[mesheryctl] Make 'system reset' context aware #2265

merged 8 commits into from Jan 22, 2021

Conversation

pottekkat
Copy link
Contributor

@pottekkat pottekkat commented Jan 19, 2021

Signed-off-by: navendu-pottekkat navendupottekkat@gmail.com

Description

This PR fixes #2248

Notes for Reviewers

This is a draft PR and is not ready for review.

Signed commits

  • Yes, I signed my commits.

Signed-off-by: navendu-pottekkat <navendupottekkat@gmail.com>
@anirudhjain75
Copy link
Member

This is still a WIP do ping me when you're partially or completely done. Or if you need my help. As far as I can see the changes are yet to be implemented

Signed-off-by: navendu-pottekkat <navendupottekkat@gmail.com>
Signed-off-by: navendu-pottekkat <navendupottekkat@gmail.com>
@pottekkat
Copy link
Contributor Author

@anirudhjain75 @leecalcote Yes, this is a WIP. I still have to implement the fetch from GitHub using the API part. I will try to get it done today.

Signed-off-by: navendu-pottekkat <navendupottekkat@gmail.com>
Signed-off-by: navendu-pottekkat <navendupottekkat@gmail.com>
Signed-off-by: navendu-pottekkat <navendupottekkat@gmail.com>
@pottekkat
Copy link
Contributor Author

@leecalcote @anirudhjain75 I have made a basic fix for this issue that seem to be working while I tested locally. I still need to setup some error handling for all possible scenarios. But this works and please check it and let me know for any changes.

@leecalcote
Copy link
Member

@navendu-pottekkat, I don't believe that this was part of the original ask, but will you consider support of the -c flag for temporarily overriding the current context? Support for this at the system level is coming. @anirudhjain75 does any consideration need to be made here in order to support -c as written here - https://github.com/layer5io/meshery/pull/2256/files#diff-3ec6eb99922674fe5326ef77839a41f6e5356441ffebb8501c2196ace27a93aaR71?

I assume not given that the system level support should provide this to all system level commands.

Signed-off-by: leecalcote <leecalcote@gmail.com>
leecalcote
leecalcote previously approved these changes Jan 21, 2021
mesheryctl/internal/cli/root/system/reset.go Outdated Show resolved Hide resolved
}

currentContext := mctlCfg.CurrentContext
currChannel := mctlCfg.Contexts[currentContext].Channel
Copy link
Member

Choose a reason for hiding this comment

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

The system level struct should be filtering any invalid values for channel and version, so confirmation of valid values shouldn't have to be considered here.

@anirudhjain75 will you confirm that this is the case?

@leecalcote leecalcote marked this pull request as ready for review January 21, 2021 06:41
@pottekkat
Copy link
Contributor Author

@navendu-pottekkat, I don't believe that this was part of the original ask, but will you consider support of the -c flag for temporarily overriding the current context? Support for this at the system level is coming. @anirudhjain75 does any consideration need to be made here in order to support -c as written here - https://github.com/layer5io/meshery/pull/2256/files#diff-3ec6eb99922674fe5326ef77839a41f6e5356441ffebb8501c2196ace27a93aaR71?

I assume not given that the system level support should provide this to all system level commands.

I think it is better to raise this as a separate issue and make a new PR

Signed-off-by: navendu-pottekkat <navendupottekkat@gmail.com>
@pottekkat
Copy link
Contributor Author

@anirudhjain75 @leecalcote I have made the changes to the logs. It looks like this now-

navendu@navendu:~/Code/meshery-dev/meshery/mesheryctl$ ./mesheryctl system reset
Meshery resetting...

Current Context: local
Channel: stable
Version: v0.4.20

Fetching default docker-compose file at version: v0.4.20...

...Meshery config (/home/navendu/.meshery/meshery.yaml) now reset to default settings.

Overriding the context using the -c flag can be made into another issue and a separate PR can be made.

@anirudhjain75 Please review and merge this PR.

image

Copy link
Contributor

@hexxdump hexxdump left a comment

Choose a reason for hiding this comment

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

instead of default settings - can we print currVersion ? or default settings of currVersion?

@pottekkat
Copy link
Contributor Author

pottekkat commented Jan 21, 2021

instead of default settings - can we print currVersion ? or default settings of currVersion?

@hexxdump We are actually printing the version. Here v0.4.20 is the version.

Fetching default docker-compose file at version: v0.4.20...

...Meshery config (/home/navendu/.meshery/meshery.yaml) now reset to default settings.

@leecalcote
Copy link
Member

@navendu-pottekkat, I don't believe that this was part of the original ask, but will you consider support of the -c flag for temporarily overriding the current context? Support for this at the system level is coming. @anirudhjain75 does any consideration need to be made here in order to support -c as written here - https://github.com/layer5io/meshery/pull/2256/files#diff-3ec6eb99922674fe5326ef77839a41f6e5356441ffebb8501c2196ace27a93aaR71?
I assume not given that the system level support should provide this to all system level commands.

I think it is better to raise this as a separate issue and make a new PR

Sounds real good. Let's do a new issue/PR.

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.

Looking good. A follow up issue on "platform" support for "docker" and for -c flag...

@leecalcote leecalcote merged commit 2b359a3 into meshery:master Jan 22, 2021
@leecalcote leecalcote added this to the v0.5.0 milestone Jan 22, 2021
@pottekkat pottekkat deleted the navendu-pottekkat/-mesheryctl]/system-reset branch January 22, 2021 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/mesheryctl CLI for Meshery
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mesheryctl] make system reset context-aware
4 participants