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

feat: default config #2067

Merged
merged 7 commits into from
Sep 4, 2023
Merged

feat: default config #2067

merged 7 commits into from
Sep 4, 2023

Conversation

markphelps
Copy link
Collaborator

Fixes: FLI-580

Change: Uses the default config values if no config file found
This is a departure from our previous approach of erroring if there is no config file

@markphelps markphelps requested a review from a team as a code owner September 1, 2023 16:29

package main

var defaultCfgPath string
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i guess another alternative is to check the OS at runtime instead of using build tags and an empty var here, anyone have a pref over one or the other?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is cool. It follows the pattern of the other things we are doing with build tags as well, so just for consistency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wdyt @GeorgeMac ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah im for that. I was going to say, as long as we warn as such. But you've done that, so all gravy to me 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah was debating on if this should be at Info level instead of Warning

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #2067 (0b991cc) into main (820f90f) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2067   +/-   ##
=======================================
  Coverage   70.41%   70.41%           
=======================================
  Files          70       70           
  Lines        6822     6822           
=======================================
  Hits         4804     4804           
  Misses       1741     1741           
  Partials      277      277           
Files Changed Coverage Δ
internal/config/config.go 87.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more


package main

var defaultCfgPath string
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah im for that. I was going to say, as long as we warn as such. But you've done that, so all gravy to me 👍

@markphelps markphelps enabled auto-merge (squash) September 4, 2023 17:57
@markphelps markphelps merged commit f743945 into main Sep 4, 2023
25 checks passed
@markphelps markphelps deleted the default-config branch September 4, 2023 18:16
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.

3 participants