Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[sonic-package-manager] first phase implementation of sonic-package-manager #1527
[sonic-package-manager] first phase implementation of sonic-package-manager #1527
Changes from 47 commits
2356869
ece3845
8bc75a7
e72f540
1a6d5d9
be4f089
2240308
9fd283d
0cbaa3e
1a99797
a80ecb0
06dfd30
725210d
f2ee39f
c724569
3dc793f
faeadcf
ae1ce45
83f2af6
d31497a
c2eacdf
801ff85
f5ba246
2dee5c6
08c9c6d
42a2665
413b44f
78afedd
f101f1f
4d03aa8
180eae3
3b98489
3280b5b
21ae66b
5548976
5a2675d
68f90f5
e7f1c59
ae5bad9
909079a
a394b27
9653c61
0e96ddb
9090a47
b2eeb59
e765d5d
24fb439
fd46c34
86bf53f
1ace0e1
6feda36
a2f7649
0d5dfcf
4c2f986
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I see it as
"name" == Feature name
"Repository" == docker-image name
"repository" is more used in the context of storage place, where multiple images could be available,
Even you say the same below "A package is a docker image, a repo may contain several docker images."
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.
Is the list going to show default-reference too?
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.
We may add if this is required
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.
How are the docker image tags going to be used here? Maybe they should be shown instead or additionally to the "Version" ?
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.
It can be added
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.
e.g. "... add cpu-report azure/sonic-cpu-report ..."
what is this "azure/sonic-cpu-report" ?
What is the purpose of default-reference ?
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 install command, takes even repo path, so wonder what is the value of this repo?
Guess we can discuss 1:1, as I guess we are not on same page.
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.
what is this "azure/sonic-cpu-report" ?
This is a repository, arbitrary example.
What is the purpose of default-reference ?
The purpose of default-reference is described in HLD - https://github.com/stepanblyschak/SONiC/blob/sonic-app-ext-3/doc/sonic-application-extention/sonic-application-extention-hld.md#sonic-package-database.
The install command, takes even repo path, so wonder what is the value of this repo?
Not sure I understand the question.
The install command takes an expression in format name==version. The reposiotory lookup takes place based on name of the package.
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.
What does "enable" mean here? Does this refer to the systemd state "enabled" or does it also trigger some other kind of configuration? If it's just systemd state, i would probably opt to use something like "--service-state [enabled|disabled]"
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.
Controls the feature state - https://github.com/Azure/SONiC/blob/master/doc/Optional-Feature-Control.md#sonic-optional-feature-control-enhancement
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 i am allowed to install "from tarball", why am i not allowed to remove a repository before removing (L8030) ?
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.
Installing from tarball will add an entry in packages.json. So still your not able to remove this record.
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.
I think a "force install" is very hard to implement, given that we do not know why the "not-forced" approach did not work. Maybe we should drop this flag and just let the user know why this did not work and how she/he can fix that.
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.
"force install" is very hard to implement - could you please give an example? Could you please take a look at its implementation?
We cannot drop it, it was a requirement.
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.
This should mention what the default level is.
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.
This help comes from https://pypi.org/project/click-log/ click decorator, I cannot modify it unless I copy and modify its code.
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.
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.
Should we also allow uninstalling multiple packages at once?
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.
It can be added
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.
Same as for install, i think force is not a good idea here. The package-manager should abstract and "protect the user" in the sense, that it does not break the system. If the user decides to replace a docker container or binary somewhere, she/he is still free to do so without the package-manger.
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 package-manager should abstract and "protect the user" in the sense, that it does not break the system.
[SB] Then the user does not use --force
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.
Personally, i find "reset" very confusing when targeting "just one package". I think a command to reset ALL packages to their default state (if possible when considering that those packages might also have installed tools on the host, which usually do not have a straight "revert" option) would be very useful thou.
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.
"packages might also have installed tools on the host, which usually do not have a straight "revert" option" - could you please give an example?
Reset multiple can be added
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.
I think "tags" might confuse people here, since it was not mentioned in the docs so far.
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 docs states about Docker and Repository and Registry, it does not look like user can be aware about it but not understand "tags in repository"
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.
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.
Is "--all" the default ? If not, what is the default ?
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.
No, it is not. --all is an option so it is not a default.
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.
Either we register the repo, in which case don't allow install/upgrade to override
Or, allow this command to accept repo
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.
Does this also stop and uninstall the corresponding service, if running? [SB] It is user responsibilty
Or would it demand, the service should be in stopped state, for remove to be success? [SB] Yes
Would it fail, if there are dependent packages installed? [SB] Yes, uninstallation will fail.
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.
Either we register the repo, in which case don't allow install/upgrade to override
Or, allow this command to accept repo
Could you please clarify which command you are refering to and what is your suggestion?
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.
Although it looks nice to humans, output like this is very hard to parse for automation tools. Maybe we can offer an option to output something like yaml, json or something similar here?
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.
Then use --plain
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.
Which format will "plain" be in ? If it is any defined format (like yaml, json or something alike), i think we wanna rename that option to "--format $FORMAT" or "--$FORMAT"
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.
package_expr ??
Does manifest carry change log ?
manifest.json.j2 does not have it
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.
Does manifest carry change log ? [SB] Yes
manifest.json.j2 does not have it. [SB] We may extend, btw, extension usually won't be in sonic-buildimage repository so they won't use manifest.json.j2
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.
Will it upgrade packages, if currently installed is lower version, w/o --skip flag?
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.
yes, please refer to HLD - https://github.com/stepanblyschak/SONiC/blob/sonic-app-ext-3/doc/sonic-application-extention/sonic-application-extention-hld.md#sonic-2-sonic-upgrade