[Banner] Added CLI commands to configure Banner and display current configuration #3021
Conversation
liat-grozovik
changed the title
|
@SviatoslavBoichuk i am missing command reference guide updates. please update it based on the new proposed CLI commands |
SviatoslavBoichuk
force-pushed
the
dev-banner-feature
branch
from
fd8f90d to
1e4346a
Compare
Added Commands to Command Reference Quide. |
SviatoslavBoichuk
force-pushed
the
dev-banner-feature
branch
3 times, most recently
from
1bc8ea7 to
85db9b5
Compare
SviatoslavBoichuk
force-pushed
the
dev-banner-feature
branch
from
85db9b5 to
42eea90
Compare
|
|
||
| config_db = ConfigDBConnector() | ||
| config_db.connect() | ||
| config_db.mod_entry(swsscommon.CFG_BANNER_MESSAGE_TABLE_NAME, 'global', |
There was a problem hiding this comment.
FEATURE table was designed for SONiC App extensions features. The Banner feature is not the case...
There was a problem hiding this comment.
I see many features are not SONiC App extensions feature there.
There was a problem hiding this comment.
I see many features are not SONiC App extensions feature there.
Sure. But we need to keep separate. In that case, it is much easier to handle. In hostcfgd we have a logic to handle FEATURE table changes. It would be overload to add additional logic to handle the banner there.
So for that, we added a new table and new subscriber in hostcfgd to handle it separately. The code looks much clearer and it is consistent over other features: NTP, RSYSLOG, SSH, etc.
There was a problem hiding this comment.
@qiluo-msft FEATURE table in general is associated with an application with a docker today. We have a dedicated application featured which listens to feature table and enables system services. We also have coppmgr using this logic to enable traps. Adding entry to feature table when there is no associated docker will lead to errors in some flows and currently it is not supported. Hence in this scenario it is advisable to use a separate table.
There was a problem hiding this comment.
@qiluo-msft please see @dgsudharsan answer. I agree with him
|
The test is failing because sonic-net/sonic-swss-common#826 should be merged first |
fastiuk
force-pushed
the
dev-banner-feature
branch
from
42eea90 to
f8f1510
Compare
fastiuk
force-pushed
the
dev-banner-feature
branch
from
f8f1510 to
262226d
Compare
|
@qiluo-msft kindly review the comments and lets close the review loop. |
Checkers are not passing because sonic-swss-common PR must be merged first |
fastiuk
force-pushed
the
dev-banner-feature
branch
2 times, most recently
from
93b6e22 to
9bdd4ca
Compare
|
@qiluo-msft conflicts were resolved. Please re-review/approve. |
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
fastiuk
force-pushed
the
dev-banner-feature
branch
from
9bdd4ca to
7a7f9f7
Compare
|
@qiluo-msft rebased with the master. Please re-review/approve. |
|
@qiluo-msft , gentle reminder :) |
|
@qiluo-msft , gentle reminder :) |
|
@qiluo-msft , gentle reminder :) |
|
@SviatoslavBoichuk , we are interested in the banner feature. can it be merged anytime soon? |
It should be merged soon. We are waiting for the review/approve |
|
@SviatoslavBoichuk can this be merged soon as it's been approved? |
|
@qiluo-msft rebased, please merge |
|
@qiluo-msft could you please merge it? It was approved by you, I fixed all conflicts and it passed the CI here |
|
@qiluo-msft gentle reminder to merge |
|
@qiluo-msft gentle reminder to merge, it is waiting for more than two weeks |
What I did
Added CLI commands for Banner feature according to HLD: sonic-net/SONiC#1361
How I did it
Added CLI commands to:
How to verify it
Manual testing
Previous command output (if the output of a command-line utility has changed)
N/A
New command output (if the output of a command-line utility has changed)
N/A