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

refactor!: Rename environment variables for the sake of consistency #402

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

FelixTing
Copy link
Member

@FelixTing FelixTing commented Dec 7, 2022

resolve #399

BREAKING CHANGE:

  • EDGEX_CONFIGURTION_PROVIDER is replaced by EDGEX_CONFIG_PROVIDER
  • EDGEX_CONF_DIR is replaced by EDGEX_CONFIG_DIR

Signed-off-by: Felix Ting [email protected]

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/go-mod-bootstrap/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

  • I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
  • I am not introducing a new dependency (add notes below if you are)
  • I have added unit tests for the new feature or bug fix (if not, why?)
  • I have fully tested (add details below) this the new feature or bug fix (if not, why?)
  • I have opened a PR for the related docs change (if not, why?) docs: Update docs for changes to environment variable naming in 3.0 edgex-docs#914

Testing Instructions

  1. Download this docker-compose file(https://github.com/edgexfoundry/edgex-compose/blob/main/docker-compose-no-secty.yml) and edit it to remove device-virtual
  2. Run EdgeX Foundry with the docker-compose file
  3. Download https://github.com/FelixTing/go-mod-bootstrap/tree/issue-399
  4. Download device-virtual (https://github.com/edgexfoundry/device-virtual-go), edit go.mod to replace github.com/edgexfoundry/go-mod-bootstrap/v2 with the one downloaded in step 3. For example
replace github.com/edgexfoundry/go-mod-bootstrap/v2 => ../go-mod-bootstrap
  1. Build a binary executable for device-virtual
make build
  1. Set environment variables, for example:
export EDGEX_CONFIG_PROVIDER=consul.http://localhost:8500
export EDGEX_CONFIG_FILE=my-config.toml
  1. Run device-virtual
  2. Logs similar to the following are expected to appear:
level=INFO ts=2022-12-07T04:43:41.025805Z app=device-virtual source=variables.go:377 msg="Variables override of '-f/-file' by environment variable: EDGEX_CONFIG_FILE=my-config.toml"
level=INFO ts=2022-12-07T04:43:41.029159Z app=device-virtual source=variables.go:377 msg="Variables override of 'Configuration Provider Information' by environment variable: EDGEX_CONFIG_PROVIDER=consul.http://localhost:8500"

New Dependency Instructions (If applicable)

@codecov-commenter
Copy link

Codecov Report

Merging #402 (a16b82a) into main (249f27d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #402   +/-   ##
=======================================
  Coverage   59.52%   59.52%           
=======================================
  Files          24       24           
  Lines        1690     1690           
=======================================
  Hits         1006     1006           
  Misses        640      640           
  Partials       44       44           
Impacted Files Coverage Δ
bootstrap/environment/variables.go 90.58% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cloudxxx8 cloudxxx8 requested a review from ejlee3 December 7, 2022 05:59
bootstrap/environment/variables.go Outdated Show resolved Hide resolved
bootstrap/environment/variables.go Outdated Show resolved Hide resolved
bootstrap/environment/variables.go Outdated Show resolved Hide resolved
@ejlee3
Copy link
Contributor

ejlee3 commented Dec 7, 2022

Commit message should be refactor!: instead of feat:. This really is a refactor of the code and the ! helps denote the breaking change.

@lenny-goodell
Copy link
Member

lenny-goodell commented Dec 7, 2022

I think we should also rename the corresponding command line flags

  • -c/--confdir to -cd/--configDir
  • -f/--file to -cf/--configFile

@FelixTing FelixTing marked this pull request as draft December 8, 2022 04:08
BREAKING CHANGE:
- `EDGEX_CONFIGURTION_PROVIDER` is replaced by `EDGEX_CONFIG_PROVIDER`
- `EDGEX_CONF_DIR` is replaced by `EDGEX_CONFIG_DIR`

Signed-off-by: Felix Ting <[email protected]>
@FelixTing FelixTing requested a review from ejlee3 December 8, 2022 06:54
@FelixTing FelixTing marked this pull request as ready for review December 8, 2022 06:54
@cloudxxx8
Copy link
Member

I think we should also rename the corresponding command line flags

  • -c/--confdir to -cd/--configDir
  • -f/--file to -cf/--configFile

the issue edgexfoundry/edgex-go#4248 is opened for this, so we won't make the related change in this PR

Copy link
Contributor

@ejlee3 ejlee3 left a comment

Choose a reason for hiding this comment

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

LGTM - I was able to see the changes with the testing instructions.

Copy link
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

LGTM

@cloudxxx8 cloudxxx8 merged commit ff25685 into edgexfoundry:main Dec 14, 2022
@FelixTing FelixTing deleted the issue-399 branch December 14, 2022 02:21
@FelixTing FelixTing changed the title feat: Rename environment variables for the sake of consistency refactor!: Rename environment variables for the sake of consistency Dec 14, 2022
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.

Environment Variable Naming Consistency
5 participants