Skip to content

[Firmware ] Run SONIC command#1793

Closed
KostiantynYarovyiBf wants to merge 1 commit intosonic-net:masterfrom
KostiantynYarovyiBf:kyarovyi/barefoot_platform_firmware
Closed

[Firmware ] Run SONIC command#1793
KostiantynYarovyiBf wants to merge 1 commit intosonic-net:masterfrom
KostiantynYarovyiBf:kyarovyi/barefoot_platform_firmware

Conversation

@KostiantynYarovyiBf
Copy link

What I did

add log Not supported firmware updates if platform is barefoot

How I did it

add raise exception if platform is barefoot

How to verify it

Run show platform firmware updates

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

fwutil/main.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

To simplify dependencies management imports should be grouped in next order:

1. Standard library imports.
2. Related third party imports.
3. Local application/library specific imports.

https://www.python.org/dev/peps/pep-0008/#imports

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting from https://www.python.org/dev/peps/pep-0008/#imports

# Correct:
import os
import sys
# Wrong:
import sys, os

Is it affect logic of work or its just a recommendation for style of code?

Copy link
Author

Choose a reason for hiding this comment

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

done

fwutil/main.py Outdated
Copy link
Contributor

@globaltrouble globaltrouble Sep 1, 2021

Choose a reason for hiding this comment

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

the expression can be simplified, to not duplicate code and simplify adding new entries:

platforms = {
  "x86_64-accton_as9516bf_32d-r0",
  "x86_64-accton_as9516_32d-r0",
  "x86_64-accton_wedge100bf_32x-r0",
  "x86_64-accton_wedge100bf_65x-r0",
}
if device_info.get_platform() in platforms :
    ...

Copy link

Choose a reason for hiding this comment

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

What if a new barefoot platform will be added?
Please use get_platform_info and check by asic_type filed. https://github.com/msosyak/sonic-buildimage/blob/2e7e7814cc71c353bef179c7f29bf00fcbac8714/src/sonic-py-common/sonic_py_common/device_info.py#L327

Copy link
Author

Choose a reason for hiding this comment

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

done

fwutil/main.py Outdated
Copy link

Choose a reason for hiding this comment

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

Suggested change
raise NameError("Firmware updates are not supported for barefoot")
raise NameError("Firmware updates are not supported by barefoot devices")

Copy link
Author

Choose a reason for hiding this comment

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

done

@KostiantynYarovyiBf KostiantynYarovyiBf force-pushed the kyarovyi/barefoot_platform_firmware branch 2 times, most recently from f323572 to 2f1eac5 Compare September 1, 2021 16:31
    What I did
    add log Not supported firmware updates if platform is barefoot

    How I did it
    add raise exception if platform is barefoot

    How to verify it
    Run show platform firmware updates
@KostiantynYarovyiBf
Copy link
Author

@jleveque please review

@KostiantynYarovyiBf
Copy link
Author

close this PR because add correct fix with another PR
sonic-net/sonic-buildimage#8690

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants