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

[distgen] Allow override of global JSON via ENV #19

Merged
merged 8 commits into from Feb 28, 2020

Conversation

@tarcinil
Copy link
Contributor

tarcinil commented Feb 12, 2020

This work allows the go generate command to have an environment variable DIST_FILE

fixes #18

Description

This work will look for an environmental variable called DIST_FILE, if it is able to find the variable, it will load the path into the globDistJson variable.

Related Issue

#18

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • [N/A] I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.
tarcinil added 2 commits Feb 12, 2020
Signed-off-by: Vern Burton <me@vernburton.com>
Signed-off-by: Vern Burton <me@vernburton.com>
@tarcinil tarcinil requested review from chef/chef-workstation-approvers as code owners Feb 12, 2020
@afiune afiune self-assigned this Feb 12, 2020
Copy link
Contributor

afiune left a comment

Great contribution @tarcinil !! A couple of things to work on but after that, I will be ready to approve and merge!

tenor-259759617

distgen/main.go Outdated Show resolved Hide resolved
distgen/README.md Show resolved Hide resolved
distgen/README.md Outdated Show resolved Hide resolved
distgen/README.md Show resolved Hide resolved
@afiune

This comment has been minimized.

Copy link
Contributor

afiune commented Feb 12, 2020

Oh, one last thing, you need to comply with DCO policy (sign off your PRs)

@tarcinil

This comment has been minimized.

Copy link
Contributor Author

tarcinil commented Feb 12, 2020

Yeah. The failed one if from applying in Github. I will amend-sign it.

Simplifying if statement

Co-Authored-By: Salim Afiune <afiune@chef.io>
Signed-off-by: Vern Burton <me@vernburton.com>
@tarcinil tarcinil force-pushed the tarcinil:feature/add-env-flag-for-dist branch from 4d53669 to 6b4878f Feb 12, 2020
Signed-off-by: Vern Burton <me@vernburton.com>
@tarcinil tarcinil requested review from afiune and mjingle Feb 13, 2020
distgen/doc.go Outdated Show resolved Hide resolved
@afiune
afiune approved these changes Feb 14, 2020
Copy link
Contributor

afiune left a comment

I am going to wait for @mjingle to verify the sentences but I would recommend doing the
explanation on a single paragraph.

README.md Suggestion

### Using an Environment Variable

The usage of an environmental variable allows for better flexibility when dealing with
CI systems. As we saw in the previous example, the 3rd argument that is passed to
`go run` is a URI, which is used to override the default JSON file for `distgen`. The
`DIST_FILE` environment performs the same functionality by overloading the default
JSON file. The usage inside of your dist files will not change, but you can pass ENV
as a part of your `go generate` command.

With the example as you have it.

doc.go Suggestion

Using an Environment Variable

The usage of an environmental variable allows for better flexibility when dealing with
CI systems. As we saw in the previous example, the 3rd argument that is passed to
"go run" is a URI, which is used to override the default JSON file for distgen. The
DIST_FILE environment performs the same functionality by overloading the default
JSON file. The usage inside of your dist files will not change, but you can pass ENV
as a part of your "go generate" command.

  DIST_FILE="https://raw.githubusercontent.com/chef/go-libs/master/distgen/tiny_glob_dist.json" go generate

The godocs format is very picky, so this would render correctly, I tested it by running a local
godoc site with the command godoc -http=":1234"

Signed-off-by: Vern Burton <me@vernburton.com>
Copy link

IanMadd left a comment

Rewrite for clarity.

The usage of an environmental variable allows for better flexibility when dealing with CI systems.
The usage of an environmental variable allows for better flexibility when dealing with
CI systems. As we saw in the previous example, the 3rd argument that is passed to
`go run` is a URI, which is used to override the default JSON file for `distgen`. The
`DIST_FILE` environment performs the same functionality by overloading the default
JSON file. The usage inside of your dist files will not change, but you can pass ENV
as a part of your `go generate` command.
Comment on lines 49 to 55

This comment has been minimized.

Copy link
@IanMadd

IanMadd Feb 27, 2020

Suggested change
The usage of an environmental variable allows for better flexibility when dealing with CI systems.
The usage of an environmental variable allows for better flexibility when dealing with
CI systems. As we saw in the previous example, the 3rd argument that is passed to
`go run` is a URI, which is used to override the default JSON file for `distgen`. The
`DIST_FILE` environment performs the same functionality by overloading the default
JSON file. The usage inside of your dist files will not change, but you can pass ENV
as a part of your `go generate` command.
An environmental variable will give you greater flexibility in CI systems.
In the previous example, the 3rd argument that is passed in to `go run` is a URI which is used to override the default JSON file for `distgen`.
The `DIST_FILE` environment performs the same function by overloading the default JSON file.
The format of your dist files will not change, but you can include the ENV in your `go generate` command.
distgen/README.md Outdated Show resolved Hide resolved
distgen/README.md Outdated Show resolved Hide resolved
distgen/README.md Outdated Show resolved Hide resolved
distgen/README.md Outdated Show resolved Hide resolved
distgen/README.md Outdated Show resolved Hide resolved
distgen/README.md Outdated Show resolved Hide resolved
Signed-off-by: Tyler Ball <tball@chef.io>

Co-Authored-By: Kimberly Garmoe <kgarmoe@chef.io>
Copy link
Member

tyler-ball left a comment

Removing some duplicates and copying the updated README into the golang

distgen/README.md Outdated Show resolved Hide resolved
distgen/README.md Outdated Show resolved Hide resolved
distgen/doc.go Outdated Show resolved Hide resolved
Signed-off-by: Tyler Ball <tball@chef.io>
Copy link
Member

tyler-ball left a comment

Fixing line wrap length

distgen/README.md Outdated Show resolved Hide resolved
distgen/doc.go Outdated Show resolved Hide resolved
Signed-off-by: Tyler Ball <tball@chef.io>
@chef chef deleted a comment from chef-expeditor bot Feb 27, 2020
@chef chef deleted a comment from chef-expeditor bot Feb 27, 2020
@chef-expeditor

This comment has been minimized.

Copy link

chef-expeditor bot commented Feb 27, 2020

📊 97.4% Code Coverage

🔄 This change neither increases nor decreases the code coverage. (master 97.4%)

0% Increase! 😅

👀 Read the uploaded HTML coverage report

Function defined at Function name Coverage
config/config.go:111: New 100.0%
config/config.go:133: App 100.0%
config/config.go:147: User 100.0%
config/config.go:160: MergeAppConfig 100.0%
config/config.go:164: MergeUserConfig 100.0%
config/config.go:168: loadAppConfig 90.9%
config/config.go:189: loadUserConfig 90.9%
config/finder.go:31: FindChefWSUserConfigFile 100.0%
config/finder.go:37: FindChefWSAppConfigFile 100.0%
config/finder.go:44: FindConfigFile 92.3%
config/finder.go:72: configFileExistsInsideDefaultDirectories 100.0%
config/workstation.go:34: ChefWorkstationDir 100.0%
credentials/credentials.go:90: ActiveProfile 100.0%
credentials/credentials.go:95: SwitchProfile 100.0%
credentials/credentials.go:112: FromViper 100.0%
credentials/credentials.go:144: NewDefault 100.0%
credentials/credentials.go:149: New 94.4%
credentials/credentials.go:184: FindCredentialsFile 100.0%
distgen/example-main/main.go:22: main 100.0%
featflag/features.go:92: init 100.0%
featflag/features.go:109: New 100.0%
featflag/features.go:134: LoadConfig 100.0%
featflag/features.go:138: ListAll 100.0%
featflag/features.go:147: String 100.0%
featflag/features.go:151: Env 100.0%
featflag/features.go:155: Key 100.0%
featflag/features.go:159: Equals 100.0%
featflag/features.go:173: Enabled 100.0%
featflag/features.go:191: valueFromConfig 100.0%
featflag/lookup.go:22: Lookup 100.0%
featflag/lookup.go:35: GetFromKey 100.0%
featflag/lookup.go:45: GetFromEnv 100.0%
total: (statements) 97.4%
@tyler-ball

This comment has been minimized.

Copy link
Member

tyler-ball commented Feb 27, 2020

@kagarmoe got your suggestions incorporated!

Copy link
Member

kagarmoe left a comment

tenor-257526346

@afiune
afiune approved these changes Feb 27, 2020
@afiune

This comment has been minimized.

Copy link
Contributor

afiune commented Feb 27, 2020

Oh, how sad, my checks are no longer green! 😭

@tyler-ball tyler-ball merged commit ca0fc30 into chef:master Feb 28, 2020
3 checks passed
3 checks passed
DCO This commit has a DCO Signed-off-by
Details
buildkite/chef-go-libs-master-verify Build #39 passed (1 minute, 49 seconds)
Details
expeditor/config-validation Validated your Expeditor config file
Details
@tyler-ball

This comment has been minimized.

Copy link
Member

tyler-ball commented Feb 28, 2020

@afiune I took it as an approval

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.