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

[platform] Add Support For Environment Variable File #5010

Merged

Conversation

tahmed-dev
Copy link
Contributor

This PR adds the ability to read environment file from /etc/sonic.
the file contains immutable SONiC config attributes such as platform,
hwsku, version, device_type. The aim is to minimize calls being made
into sonic-cfggen during boot time.

singed-off-by: Tamer Ahmed tamer.ahmed@microsoft.com

- Why I did it
Added ability to read environment var file

- How I did it
New code that detect presence of such file during bootstrapping an image, the file is then moved to /etc/sonic

- How to verify it
This is infra work and image should boot normally in the absence of such file.

- Description for the changelog

@tahmed-dev tahmed-dev force-pushed the taahme/add-support-for-sonic-envvars branch from 5261f5f to 9b03c94 Compare July 21, 2020 17:00
@tahmed-dev tahmed-dev marked this pull request as ready for review July 21, 2020 17:01
@tahmed-dev tahmed-dev requested a review from jleveque July 21, 2020 18:25
@tahmed-dev tahmed-dev changed the title [platform] Add Support For Environment Variable [platform] Add Support For Environment Variable File Jul 23, 2020
Copy link
Contributor

@pavel-shirshov pavel-shirshov left a comment

Choose a reason for hiding this comment

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

Can you please review some of my comments.
The changes are look good, I'd suggest to move all include operations to the first lines of the file. Like include first and than use the values here and there

pavel-shirshov
pavel-shirshov previously approved these changes Jul 24, 2020
@lguohan
Copy link
Collaborator

lguohan commented Jul 24, 2020

there is a build failure, looks like related to your change.

https://sonic-jenkins.westus2.cloudapp.azure.com/job/broadcom/job/buildimage-brcm-all-pr/4171/console

@tahmed-dev
Copy link
Contributor Author

there is a build failure, looks like related to your change.

https://sonic-jenkins.westus2.cloudapp.azure.com/job/broadcom/job/buildimage-brcm-all-pr/4171/console

Looking into it this failure.

This PR adds the ability to read environment file from /etc/sonic.
the file contains immutable SONiC config attributes such as platform,
hwsku, version, device_type. The aim is to minimize calls being made
into sonic-cfggen during boot time.

singed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
1. move sonic-environment relocation from union-mount to rc.local
2. remove SONIC_HOSTNAME envvar
@tahmed-dev tahmed-dev force-pushed the taahme/add-support-for-sonic-envvars branch from c68086d to 1ed0981 Compare July 27, 2020 23:38
Comment on lines +10 to +11
PLATFORM=${PLATFORM:-`sonic-cfggen -H -v DEVICE_METADATA.localhost.platform`}
HWSKU=${HWSKU:-`sonic-cfggen -d -v 'DEVICE_METADATA["localhost"]["hwsku"]'`}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest using the same syntax for these variables. Either change former to 'DEVICE_METADATA["localhost"]["platform"]' or change latter to DEVICE_METADATA.localhost.hwsku

Comment on lines +16 to +17
PLATFORM=${PLATFORM:-`sonic-cfggen -H -v DEVICE_METADATA.localhost.platform`}
HWSKU=${HWSKU:-`sonic-cfggen -d -v 'DEVICE_METADATA["localhost"]["hwsku"]'`}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest using the same syntax for these variables. Either change former to 'DEVICE_METADATA["localhost"]["platform"]' or change latter to DEVICE_METADATA.localhost.hwsku

Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

I left two minor suggested syntactical changes for pre-existing code. In the interest of getting this merged and not waiting for a new cycle of check builds, I'm approving this PR; we can clean up the syntax in a future PR.

@tahmed-dev tahmed-dev merged commit 7872b4e into sonic-net:master Aug 1, 2020
abdosi pushed a commit that referenced this pull request Sep 28, 2020
* [platform] Add Support For Environment Variable

This PR adds the ability to read environment file from /etc/sonic.
the file contains immutable SONiC config attributes such as platform,
hwsku, version, device_type. The aim is to minimize calls being made
into sonic-cfggen during boot time.

singed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
santhosh-kt pushed a commit to santhosh-kt/sonic-buildimage that referenced this pull request Feb 25, 2021
* [platform] Add Support For Environment Variable

This PR adds the ability to read environment file from /etc/sonic.
the file contains immutable SONiC config attributes such as platform,
hwsku, version, device_type. The aim is to minimize calls being made
into sonic-cfggen during boot time.

singed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants